Skip to content

only apply user plugins to loader#111

Merged
madyankin merged 8 commits intomadyankin:masterfrom
tjenkinson:only-apply-plugins-for-loader
Aug 20, 2020
Merged

only apply user plugins to loader#111
madyankin merged 8 commits intomadyankin:masterfrom
tjenkinson:only-apply-plugins-for-loader

Conversation

@tjenkinson
Copy link
Copy Markdown
Contributor

The input css has already been processed by any plugins before postcss-module, and the output will be processed by any plugins after. We shoud not run all the user plugins again.

However, for anything that is loaded in by the loader (such as from composes), we should run the plugins that were listed before postcss-modules, so that the css that is brought in can catch up.

Does this make sense? It fixes the problem for us.

Think this fixes #70

Tom Jenkinson and others added 2 commits August 14, 2020 17:28
The input `css` has already been processed by any plugins before `postcss-module`, and the output will be processed by any plugins after. We shoud not run all the user plugins again.

However, for anything that is loaded in by the loader (such as from `composes`), we should run the plugins that were listed before `postcss-modules`, so that the css that is brought in can catch up.
@tjenkinson tjenkinson force-pushed the only-apply-plugins-for-loader branch from cf66e43 to aa114f5 Compare August 14, 2020 22:05
@tjenkinson tjenkinson force-pushed the only-apply-plugins-for-loader branch from aa114f5 to 3cc479a Compare August 15, 2020 11:31
@madyankin
Copy link
Copy Markdown
Owner

Thanks for the PR! Sorry, didn't noticed the PR earlier. I'm pretty loaded now, will take a look in a couple of days.

@tjenkinson
Copy link
Copy Markdown
Contributor Author

No worries. Thanks!


const plugins = [
autoprefixer,
postcss.plugin(
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you extract this test in a separate test case to not mess up the other tests?

Copy link
Copy Markdown
Contributor Author

@tjenkinson tjenkinson Aug 20, 2020

Choose a reason for hiding this comment

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

can do but I thought this was a good thing to assert for all the cases?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I made it a new test for all the cases. This ok?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yeah, totally ok

Copy link
Copy Markdown
Owner

@madyankin madyankin left a comment

Choose a reason for hiding this comment

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

Thanks! Merging


const plugins = [
autoprefixer,
postcss.plugin(
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yeah, totally ok

@madyankin madyankin merged commit d1c997f into madyankin:master Aug 20, 2020
@madyankin
Copy link
Copy Markdown
Owner

Published in 3.2.1

@tjenkinson
Copy link
Copy Markdown
Contributor Author

Awesome thanks for the quick review! :)

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.

other plugin invoke twice

2 participants