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

chore(cssnano-preset-default): update css-declaration-sorter #1377

Merged
merged 3 commits into from Mar 30, 2022

Conversation

ludofischer
Copy link
Collaborator

Drop timsort dependency and fix issues with imports. Changelog

@ludofischer ludofischer force-pushed the update-css-declaration-sorter branch from 0c0b765 to eebdc2b Compare March 27, 2022 12:09
@Siilwyn
Copy link
Contributor

Siilwyn commented Mar 27, 2022

Can I help with this? Seems that something is wrong with the TS type?

Edit: ah I export a Plugin type which doesn't require the postcss property but you expect a PluginCreator which does...

@ludofischer ludofischer force-pushed the update-css-declaration-sorter branch from eebdc2b to 47f9bbd Compare March 27, 2022 12:18
@ludofischer
Copy link
Collaborator Author

Can I help with this? Seems that something is wrong with the TS type?

Yes, I think I know what the problem is. cssnano expects the plugins to conform to the postcss PluginCreator interface: https://github.com/postcss/postcss/blob/e5c10e009717c14c00b03867344eaefe960968da/lib/postcss.d.ts#L196

which means the function that returns the plugin must have the postcss property set to true.

css-declaration-sorter does set the property in JS:
https://github.com/Siilwyn/css-declaration-sorter/blob/82461efa9d49d44b611f2d53e85ab6988b5dc41c/src/main.mjs#L37
but then it's not declared in the TypeScript types.

@Siilwyn
Copy link
Contributor

Siilwyn commented Mar 27, 2022

I see! Just pushed 6.2.2 which uses PluginCreator, I also reverted the typing to export the default CommonJS entry.

Since having a named export and default export for both ESM and CJS in a TS type definition file is not possible, I think I'll push a major bump later so I can just use a named export. 🤔

Sidenote, I see @types/css-declaration-sorter is also a dependency which can be removed.

@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2022

Codecov Report

Merging #1377 (84d89bf) into master (28994a5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1377   +/-   ##
=======================================
  Coverage   97.65%   97.65%           
=======================================
  Files         123      123           
  Lines        9958     9958           
=======================================
  Hits         9724     9724           
  Misses        234      234           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28994a5...84d89bf. Read the comment docs.

@ludofischer ludofischer changed the title chore(cssnano-preset-default): update css-declaratio-sorter chore(cssnano-preset-default): update css-declaration-sorter Mar 27, 2022
@ludofischer
Copy link
Collaborator Author

I see! Just pushed 6.2.2 which uses PluginCreator, I also reverted the typing to export the default CommonJS entry.

Thank! Seems fine now.

@ludofischer ludofischer force-pushed the update-css-declaration-sorter branch from 3b1c27f to 6edbd5a Compare March 28, 2022 10:47
@Siilwyn
Copy link
Contributor

Siilwyn commented Mar 28, 2022

There is still an override in the site package.json right?

https://github.com/cssnano/cssnano/blob/master/site/package.json#L25-L28

(don't have much experience with pnpm)

@ludofischer
Copy link
Collaborator Author

There is still an override in the site package.json right?

  1. you're right
  2. it had no effect anyway since it's getting cssnano from the parent directory
  3. the playground seems to be still broken with the latest css-declaration-sorter
Cannot find module '../orders/alphabetical.cjs'

@Siilwyn
Copy link
Contributor

Siilwyn commented Mar 28, 2022

That is odd, the CJS build inlines the orders so this seems like old code?

Or Webpack is loading the ESM build and convers imports to cjs?

Thoughts?

@ludofischer
Copy link
Collaborator Author

That is odd, the CJS build inlines the orders so this seems like old code?

Or Webpack is loading the ESM build and convers imports to cjs?

Thoughts?

Works fine now after deleting all node_modules. I think webpack was still bundling the previous css-declaration-sorter version lying around in the pnpm cache. Might be an issue with webpack's caching.

Copy link
Contributor

@Siilwyn Siilwyn left a comment

Choose a reason for hiding this comment

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

Looks good! ✨

@ludofischer ludofischer merged commit e2b3f2d into master Mar 30, 2022
@ludofischer ludofischer deleted the update-css-declaration-sorter branch March 30, 2022 10:18
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

4 participants