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

New CSS pipeline based on @parcel/css #7538

Merged
merged 15 commits into from
Jan 11, 2022
Merged

New CSS pipeline based on @parcel/css #7538

merged 15 commits into from
Jan 11, 2022

Conversation

devongovett
Copy link
Member

This replaces most of the CSS pipeline with a new version based on @parcel/css. In particular, CSS modules, dependency analysis, auto prefixing, preset-env style transpilation, tree shaking, and minification are handled. PostCSS can still be used to handle custom plugins etc. if needed.

It's mostly working, but we still need to determine is the exact rollout plan for this so we don't break existing setups. A few notes:

  • Currently, I made a new transformer plugin that lives in parallel with @parcel/transformer-css but ideally it would replace it if we determine the new version is stable enough. We could release them separately at first and let people opt in, or we could just go for it. I'm not sure...
  • In this PR, I've changed the PostCSS transformer to only handle "legacy" CSS modules, which contain syntax that isn't supported by @parcel/css. In particular, @value and the non-function :global/:local selector syntax. Otherwise, CSS modules are handled by the new transformer. If we want an interim rollout plan where the two CSS transformers exist separately for now, then we'll need to roll this back or have some other way to ensure that CSS modules still work in the meantime even without the new transformer.
  • Can we replace the default CSS minifier without breaking changes? What if projects have cssnano config?

Please let me know your thoughts on the above questions!

@height
Copy link

height bot commented Jan 9, 2022

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@Shinyaigeek
Copy link
Contributor

Shinyaigeek commented Jan 9, 2022

@devongovett

Great work 🎉 I am really looking forward to the release of parcel-bundler with more speedy CSS processing 😍 .

Currently, I made a new transformer plugin that lives in parallel with @parcel/transformer-css but ideally it would replace it if we determine the new version is stable enough. We could release them separately at first and let people opt-in, or we could just go for it. I'm not sure…

Is this “a new transformer plugin” is parcel/transformer-css2”, right? If so, I think providing an option to use a new CSS transformer plugin or the former one to users with an experimental flag, like —experimental-parcel-css`, and collecting reports from users will lead to more stable migration. For example, when Next.js starts to use swc from v11.1.0, Next.js provided an experimental configuration to allow users to decide whether they use swc loader and minifier in order to introduce swc progressively.

FYI: vercel/next.js#27664

Can we replace the default CSS minifier without breaking changes? What if projects have cssnano config?

IMHO, cssnano can switch on/off of each feature with the configuration file, but I feel that @parcel/css’s function to specify the browser target can cover the use case enough, so I think it's enough to report a warning if the project root has cssnano.config.js when a new CSS transformer plugin is used.

Since a user can also use user-specific plugins in the cssnano’s CSS processing with the cssnano configuration file, it may be good to implement a function that can use some plugins written in JavaScript or Rust in @parcel/css.

I love both parcel/css and parcel bundler and I am watching these, but there may be some context that I have overlooked. I'm sorry if I overlooked some prerequisite context 🙇 I hope I can provide some seed for discussion and move the discussion forward.

@devongovett
Copy link
Member Author

@Shinyaigeek thanks for the feedback!

Is this “a new transformer plugin” is parcel/transformer-css2”, right?

Right. This way, you can opt in by making a .parcelrc like this:

{
  "extends": "@parcel/config-default",
  "transformers": {
    "*.css": ["@parcel/transformer-css2"] // or prepend @parcel/transformer-postcss if still needed
  },
  "optimizers": {
    "*.css": ["@parcel/optimizer-css"]
  }
}

It's not the most ideal though because you'd need to change your config again once @parcel/transformer-css2 is deprecated and becomes the default in @parcel/transformer-css.

—experimental-parcel-css

A CLI flag is a bit more complex because it requires a change to Parcel core, when really this is only changing plugins. I thought of using an environment variable also, but that's a bit annoying. Not sure...

@devongovett
Copy link
Member Author

Ok, renamed it to @parcel/transformer-css-experimental, and also copied the old CSS modules transformer into the old CSS transformer so that the PostCSS transformer works with either the old or new CSS transformer. Finally, made the tests pass against both versions of the pipeline.

@@ -3971,7 +3971,7 @@ describe('cache', function () {
let pluginContents = await overlayFS.readFile(plugin, 'utf8');
await overlayFS.writeFile(
plugin,
pluginContents.replace('green', 'blue'),
Copy link
Member Author

Choose a reason for hiding this comment

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

@parcel/css prints this as #00f, so had to change the color to something that prints the same in both transformers

@devongovett devongovett marked this pull request as ready for review January 11, 2022 04:21
@parcel-benchmark
Copy link

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 2.51s +17.00ms
Cached 398.00ms -10.00ms

Cold Bundles

Bundle Size Difference Time Difference
dist/legacy/index.c1bc86aa.css 94.00b +0.00b 1.34s -83.00ms 🚀
dist/modern/index.57a95cbe.css 94.00b +0.00b 1.34s -83.00ms 🚀

Cached Bundles

No bundle changes detected.

React HackerNews ✅

Timings

Description Time Difference
Cold 11.89s +84.00ms
Cached 534.00ms +4.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

AtlasKit Editor ✅

Timings

Description Time Difference
Cold 1.30m -472.00ms
Cached 1.68s -6.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Three.js ✅

Timings

Description Time Difference
Cold 8.32s -366.00ms
Cached 445.00ms -20.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

@@ -58,7 +58,11 @@ export default (new Packager({

queue.add(() => {
// This condition needs to align with the one in Transformation#runPipeline !
Copy link
Member

Choose a reason for hiding this comment

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

This condition needs to align with the one in Transformation#runPipeline

Probably not anymore with that change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup

@devongovett devongovett merged commit dbe2f10 into v2 Jan 11, 2022
@devongovett devongovett deleted the css branch January 11, 2022 20:24
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.

4 participants