Skip to content

Expose 'raw' option for css in types#62

Merged
Ffloriel merged 2 commits into
FullHuman:masterfrom
fpapado:allow-css-raw-selector
Mar 11, 2018
Merged

Expose 'raw' option for css in types#62
Ffloriel merged 2 commits into
FullHuman:masterfrom
fpapado:allow-css-raw-selector

Conversation

@fpapado

@fpapado fpapado commented Mar 11, 2018

Copy link
Copy Markdown
Contributor

Hello again! I thought to split this into a separate one, in case there is any iteration needed.

Proposed changes

The code and tests show that it is possible to specify a 'raw' option for css:

    css: [
      {
        raw: style,
        extension: 'css' // Technically not needed; see below
      }
    ],

Existing code segment with functionality:

cssContent = option.raw

Test using it:
https://github.com/FullHuman/purgecss/blob/master/__tests__/purgecssDefault.test.js#L323

However, the type interface does not expose it as an option. Thus, if you're using Typescript or Flow and try to use the raw option for css, an error is raised:

  Type '{ content: { raw: string; extension: string; }[]; css: { raw: string; extension: string; }[]; ext...' is not assignable to type 'Options'.
    Types of property 'css' are incompatible.
      Type '{ raw: string; extension: string; }[]' is not assignable to type 'string[]'.
        Type '{ raw: string; extension: string; }' is not assignable to type 'string'.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • [~] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

NOTE:
I went with RawContent as the option for consistency with the content interface. This type is technically true, but the extension field is not used by the code (since it would be css anyway); this is seen in

cssContent = option.raw

I can change it to an explicit type if needed:

type RawCss = {
  raw: string
}

For that matter, I have also not touched the code. Changing the cssOptions: Array<any> in getCssContents would pair well:

getCssContents(cssOptions: Array<string | RawCss>, cssSelectors: Set<string>): Array<ResultPurge> {

// Or

getCssContents(cssOptions: Array<string | RawContent>, cssSelectors: Set<string>): Array<ResultPurge> {

None of these changes would be breaking.

Let me know if any of these alternatives sound better, and I'll get to them :)

The code and tests both show that it is possible, but the
type interafce did not include it as an one.
@Ffloriel

Copy link
Copy Markdown
Member

Thank you very much for reviewing the types of Purgecss.
I'll review your PRs shortly and merge them.

@Ffloriel Ffloriel self-requested a review March 11, 2018 12:06

@Ffloriel Ffloriel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Ffloriel Ffloriel merged commit b1f01b2 into FullHuman:master Mar 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants