Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose 'raw' option for css in types #62

Merged
merged 2 commits into from Mar 11, 2018

Conversation

fpapado
Copy link
Contributor

@fpapado fpapado commented Mar 11, 2018

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
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
Copy link
Member

@Ffloriel Ffloriel left a comment

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.

None yet

2 participants