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

Improve CSS support in server components #1843

Merged
merged 24 commits into from
Jul 20, 2022
Merged

Improve CSS support in server components #1843

merged 24 commits into from
Jul 20, 2022

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Jul 14, 2022

Description

Closes #953
Closes #943 (if vanilla-extract-css/vanilla-extract#751 is merged)

In #932 and other PRs we added support to import CSS Modules in server components. That approach works fine but requires a lot of code transformations and only works for default exports (server components in named exports do not include styles).

This PR adds a new implementation that doesn't need to transform code. Instead, it collects emitted style files from Vite's module graph (in dev) and Rollup output info (in prod) and extracts them in a single CSS file. Considerations:

Pros:

  • CSS Modules work like before, but it also applies styles to named exports, not only default exports.
  • Pure CSS is also supported (import './styles.css'), and the styles will be inlined in the DOM.
  • Any CSS-in-JS at build time that emits CSS files can now be supported (e.g. we just need to change a line in vanilla-extract plugin to make it work).
  • RSC responses are now smaller because they don't include inlined styles anymore.
  • Reduces style duplication when using the same modules in multiple components.

Cons:

  • All the styles imported in the app are extracted in a single CSS asset, which means there's no split per route.

Additional context

This new feature is added behind an experimental.css flag so that we don't break existing project that use CSS Modules. Once we make sure some users start using this without issues, we can make this the default behavior (APIs are the same so it shouldn't be a breaking change).


Before submitting the PR, please make sure you do the following:

  • Read the Contributing Guidelines
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123)
  • Update docs in this repository according to your change
  • Run yarn changeset add if this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning

@frandiox frandiox requested a review from a team July 14, 2022 07:43
@github-actions

This comment has been minimized.

Copy link
Contributor

@blittle blittle left a comment

Choose a reason for hiding this comment

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

Amazing! 🥳

@benjaminsehl
Copy link
Member

Dumb question, but how would one turn on this flag? What's the copypasta?

Copy link
Contributor

@lordofthecactus lordofthecactus left a comment

Choose a reason for hiding this comment

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

@frandiox, amazing. I reviewed the code although I wanted to debug it and had some issues.
A couple of comments:

  • Should we be adding tests for this?
  • Would it make sense to add dev docs for this? e.g. in separate file or beginning of the plugin on the behavior in SSR, SCR, client side, dev vs prod.

@frandiox
Copy link
Contributor Author

Dumb question, but how would one turn on this flag? What's the copypasta?

It's like in the modified vite.config.js file in the commits:

// vite.config.js
// ...
plugins: [hydrogen({experimental: {css: 'global'}})]

Once some of our CSS Modules users test this in their projects we can enable it by default 👍

Should we be adding tests for this?

It's actually tested. I've enabled this code path in playground/server-components and there are tests for both vanilla/pure CSS and CSS Modules.

Would it make sense to add dev docs for this? e.g. in separate file or beginning of the plugin on the behavior in SSR, SCR, client side, dev vs prod.

I started writing docs but then thought that, since this is a very experimental feature, perhaps we could release it without docs first to make sure there are no breaking changes for CSS Module users (we can contact some of them directly to try this out). Then, once we are sure the global option is safe for CSS Modules, we can make it the default and remove the whole experimental.css option and the old css-modules-rsc plugin.

Having a second thought, perhaps we should just remove the whole experimental.css option and make an experimental release instead 🤔

@frandiox
Copy link
Contributor Author

Todos:

  • Fix e2e on Windows
  • Check HMR issues
  • Optional: extract generated CSS in a file instead of inlining it

@frandiox
Copy link
Contributor Author

Alright I think this is finished. Changes since the last review:

  • Fixed HMR.
  • Support Sass/Stylus/Less.
  • Ensured styles are not duplicated in prod.
  • Extract styles in a separate CSS file instead of inlining them in the DOM.

@rennyG could you please check the updated docs?

Copy link
Contributor

@rennyG rennyG left a comment

Choose a reason for hiding this comment

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

Gold standard, @frandiox! Some nitty suggestions for dev docs style.

docs/framework/css-support.md Outdated Show resolved Hide resolved
docs/framework/css-support.md Outdated Show resolved Hide resolved
docs/framework/css-support.md Outdated Show resolved Hide resolved
docs/framework/css-support.md Outdated Show resolved Hide resolved
docs/framework/css-support.md Outdated Show resolved Hide resolved
packages/hydrogen/src/framework/types.ts Show resolved Hide resolved
Co-authored-by: Ren Chaturvedi <63201605+rennyG@users.noreply.github.com>
Co-authored-by: Ren Chaturvedi <63201605+rennyG@users.noreply.github.com>
@diemonster
Copy link

@frandiox how experimental is this feature at this point? It works wonderfully for me locally, but it breaks CSS for production builds (with no relevant errors that I can find during the build process). Happy to supply any logs or extra info you may need. It made a very drastic improvement for page loads locally, so I'm more than happy to help.

FWIW i'm using https://github.com/MathiasGilson/tailwind-styled-component along w Tailwind

@frandiox
Copy link
Contributor Author

@diemonster It seems to be quite stable and there are other devs using this in production.
Can you check if this is the problem you're having with Tailwind? Shopify/hydrogen#2007

@diemonster
Copy link

@frandiox yes, that was the issue. Note to anyone else that stumbles on this: you will need to load any additional CSS files in App.server.tsx as well, particularly custom fonts if you were loading them implicitly

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.

Improve CSS-in-JS strategy Exploration: Support Vanilla Extract CSS
7 participants