-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fix postcss modules composes imports #2642
Fix postcss modules composes imports #2642
Conversation
4a6222d
to
9bccd37
Compare
@DeMoorJasper @devongovett I consider this one ready now, I am happy to receive feedback! There is still one test failing, as far as I can see it is not related to my changes and also fails in master at the moment, therefore, I ignored it for now. Some further notes I would like to share:
The only chance I see to archive this without a custom loader would be to extend the Bundle to share information between assets as its implemented for I guess this is a good compromise between complexity and performance and when using css only it should still be possible to change the |
43853dd
to
e3be9aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mentioned only one test failed but it seems a lot of tests are failing, could you resolve this?
Ahhhh, finally I understand the CI, never found the "test" tab and always tried to check the logs... Sorry for the circumstances! |
997153f
to
bd86ff6
Compare
bd86ff6
to
7e47b8d
Compare
Okay, the pipeline is now finally green. Took a bit longer as one of the tests displayed an issue with the default file system loader on windows, which was most likely there before this PR: See 7e47b8d for the fix as well as css-modules/css-modules-loader-core#230 for the original issue. |
…o bugfix/css-modules-composes-no-duplicate-classes
I just made a small clean up for the Postcss loader and merge the master branch. Maybe one more note: My team is using this for one week now without any issues and we also shipped it to production. It reduces the CSS bundle size by ~25% for us as we don't have duplicate classes within the bundle anymore, further it solved all issues we had with those duplicate CSS rules (caused by the changed specificity). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing works, thanks for this. Looks like it adds a lot of reliability to how parcel handles composes. Also great that you added so much tests to prevent issues with this in the future :)
↪️ Pull Request
The PR fixes different issues with postcss composes when using the import syntax, e.g.
composes: my-class from './another-file.css';
:composes: my-class from '~another-file.css
throws an error because the file was not found (no ticket available)Sorry for the big PR, I started fixing #2592 and found out that the issues are really closely related and it would not make sense to solve them separately (I drifted into a wrong direction when only thinking about one issue). Anyhow, all fixes are within separate commits (see the first 3).
Fixes: #2592, fixes #2501
💻 Examples
See the example in #2592 and #2501.
🚨 Test instructions
See the example in #2592 and #2501. I also tried to cover everything in integration tests.
✔️ PR Todo