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

feat: CSS Modules support #4852

Merged
merged 95 commits into from
Jan 4, 2023
Merged

feat: CSS Modules support #4852

merged 95 commits into from
Jan 4, 2023

Conversation

markdalgleish
Copy link
Member

@markdalgleish markdalgleish commented Dec 13, 2022

Closes: #4632

  • Docs
  • Tests

Note that docs will definitely some need more holistic changes once we've removed the feature flag and added more bundling features, but this is PR contains a minimal set of updates to document this change.

I ultimately went with postcss-modules as it's the most commonly used implementation in the wild and we're aiming to support people migrating from other tools. We may use other implementations alongside this in the future if it helps with performance and we can detect that unsupported features aren't being used.

Testing Strategy

I've added a Playwright test suite that checks each CSS Modules feature individually since they have different levels of support with different implementations. I've ensured that all tests are decoupled from the generated class name format since it can differ between CSS Modules implementations.

I've also added some tests to catch potential issues in the future (duplicated hashes, duplicated rules) if we want to migrate to another implementation or change our CSS minification strategy.

Since there was already a deterministic build output test, I've added some CSS Modules files to the fixture to ensure hashes are stable.

Kudos

Big shout out to @chaance for his prior work on #2489 which really helped in this implementation.

@lili21
Copy link
Contributor

lili21 commented Dec 21, 2022

is that mean all module stylesheet will be bundle into one css file?

@lili21
Copy link
Contributor

lili21 commented Dec 21, 2022

is that mean all module stylesheet will be bundle into one css file?

I noticed that the RFC already mentioned it. Ignore me.

Copy link
Collaborator

@chaance chaance left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few minor suggestions and questions. Really stoked about the approach so far 👍

docs/guides/migrating-react-router-app.md Outdated Show resolved Hide resolved

<docs-warning>This feature is unstable and currently only available behind a feature flag. We're confident in the use cases it solves but the API and implementation may change in the future.</docs-warning>

<docs-warning>Since all CSS Modules styles end up in a single CSS file at the end of the build, we recommend that you only use this styling approach for smaller applications, or sparingly within larger applications. If you're concerned about bundle size, you should probably look at [Tailwind](#tailwind-css) or [Regular Stylesheets](#regular-stylesheets) instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm actually not sure that we need this warning. I think a single module bundle is probably fine for decently large apps since it's easy to cache, and CSS files are rarely a bottleneck before some pretty serious scale. I think we can talk about the tradeoffs without making it look scary.

Copy link
Contributor

@cliffordfajardo cliffordfajardo Dec 21, 2022

Choose a reason for hiding this comment

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

It does sound a little scary if someone were evaluating remix and saw this
Not discounting that the warning isn't true. Maybe there are strategies for reducing the bundle size like minifying the CSS.

just noticed out production CSS output isn't compressed in our app 😅
Remix doesn't minify CSS by default?
Not a big deal for our internal tool or web UI, but can be for public facing UIs were download speed matters

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remix doesn't minify CSS by default?

No, Remix doesn't process CSS by default at all (aside than hashing the filename for effective versioning/caching). This is explained in the same doc.

That said, CSS Modules is opening the door a bit on this bigger question. We should absolutely be able to minify the production build of your CSS Modules bundle IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a single module bundle is probably fine for decently large apps since it's easy to cache

But change any style will break the cache.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But change any style will break the cache.

As it should! I'm not saying there are no potential perf tradeoffs here, just that we shouldn't overstate them because:

CSS files are rarely a bottleneck before some pretty serious scale

Copy link
Member Author

@markdalgleish markdalgleish Jan 2, 2023

Choose a reason for hiding this comment

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

We should absolutely be able to minify the production build of your CSS Modules bundle IMO.

The generated CSS is already being minified since it's being bundled by esbuild honouring the same minification settings as the JS output.

docs/guides/styling.md Outdated Show resolved Hide resolved
docs/guides/styling.md Outdated Show resolved Hide resolved
docs/guides/styling.md Outdated Show resolved Hide resolved
packages/remix-dev/compiler/compileBrowser.ts Outdated Show resolved Hide resolved
packages/remix-dev/compiler/compileBrowser.ts Outdated Show resolved Hide resolved
packages/remix-dev/compiler/compileBrowser.ts Outdated Show resolved Hide resolved
packages/remix-dev/compiler/compileBrowser.ts Show resolved Hide resolved
packages/remix-css-bundle/README.md Show resolved Hide resolved
@xalechez
Copy link

xalechez commented Jan 3, 2023

Hey Mark! Great stuff here, glad to see CSS modules finally coming to Remix. My question here is about third-party libraries support. We import a component library that uses CSS modules via require syntax as it's transpiled to CJS. Is this also supported with your solution? In the past we had this issue with several esbuild CSS modules plugins and it was a big blocker for us with Remix

@markdalgleish
Copy link
Member Author

@xalechez Is the component library open source? I know you'd at least need to add the package to the serverDependenciesToBundle array, but other than that I'm not sure what the issue would be.

@markdalgleish markdalgleish merged commit 3d58b0a into dev Jan 4, 2023
@markdalgleish markdalgleish deleted the markdalgleish/css-modules branch January 4, 2023 00:10
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Jan 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

🤖 Hello there,

We just published version v0.0.0-nightly-2c398e4-20230104 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants