-
Notifications
You must be signed in to change notification settings - Fork 566
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
[RFC] feat: add rollup-plugin-include-css #4609
Conversation
|
size-limit report 📦
|
cc @primer/engineer-reviewers would love your feedback if you have a sec this week! Will also bring this up in our working session 👀 |
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.
I really like the idea, especially when we compare it with the alternatives, I think this is a good way to go and thanks for writing a roll up plugin from scratch, this is so cool 🔥
- Do you think adding some tests in the plugin and adding it to the CI help with the maintenance? I feel it might be less cryptic if the plugin tests fail when there is an issue rather than primer/react build step fails - curious to hear your thoughts 🙌🏻
- I assume adding this plugin wouldn't impact the component that don't have the css modules yet?
- Do you think we should open source it under Primer in a separate repo or would it make more sense to keep it in the monorepo here?
@@ -0,0 +1,45 @@ | |||
# rollup-plugin-include-css |
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.
Ohh, you wrote this plugin!! I totally missed that point 😄 This is so cool!! 🔥
Happy to take a look and see how hard this would be 👀 Always down for adding testing 👍
The components that don't import css will not be effected but if the components are in the same entrypoint then it might impact them 😞 This would be because when trying to resolve the component that does not use CSS modules it would find a file that does in the same entrypoint and could potentially error out if what they are using does not understand importing CSS files
I think it would make sense to keep it in the monorepo so that we don't have to worry about publishing the package. It'd be great if we could keep it private for now and if we need to publish it we could either do so from here or move it to a separate project if that makes sense. |
Opened up: #4650 to add the rollup plugin to the project 👀 Going to close out this RFC! |
Summary
This Pull Request is an experiment which adds a custom Rollup plugin,
rollup-plugin-include-css
, into Primer React so that we can ship generated CSS files alongside components so that consumers only include the CSS they use.Motivation
As we move towards CSS Modules, it's important that we prioritize the overall size of a component and its styles. The current behavior when using CSS Modules along with Rollup is that there will be a single CSS file emitted. This would create a tension where someone using
@primer/react
would get all the styles for every component instead of only the styles for components they use. Over time, the size of this CSS bundle will grow even if the styles for specific components does not.This approach looks to make it so that a consumer of
@primer/react
only uses the styles they need for the components they import.Detailed design
The main addition here is through a custom rollup plugin,
rollup-plugin-include-css
, which will go through the following steps:With this approach, we know that if the classes for a CSS Module file are imported then the associated styles for that file are also imported.
On the library side, the package only needs to import and use this plugin in their rollup config. It includes support for custom ostcss and postcss-modules options and will emit the CSS files relative to their position in the source directory. For example:
Here,
Blankslate.module.css.js
will include the CSS Modules classes and will also importBlankslate-c90a59c9.css
which contains the CSS that corresponds to those classes.In addition, the library should specify that these files contain side-effects.
Drawbacks
The main drawback is that this is a custom rollup plugin that our team will need to maintain. Since we aren't familiar with writing rollup plugins, there could be potential issues down the road with maintaining it.
It also isn't immediately clear how to address things like imports between CSS files, or if we will run into ordering problems if component CSS is imported in different orders.
Alternatives
Alternatives to this approach include:
Unresolved questions
Questions
What's the impact downstream for consumers with this RFC?
Hopefully nothing! 🤞 Folks should be able to import components and use them the way they normally do. The only caveat to this is that they should make sure that their bundler (whether that's webpack, vite, parcel, next, etc) can in interpret CSS imports from JavaScript files.
What does the generated CSS look like?
This is something that is configurable through CSS Modules and can be changed over time. Details for the parts we can use are available here. One option could be to do something like:
Which would give you something like:
This would indicate that the styles for this are in the Blankslate file under the heading class.