-
-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
Add support for CSS Modules with explicit filename - [name].module.css #2285
Conversation
I have spent quiet a while investigating the failing build issue and haven't been able to resolve it. When I use this branch to create an app and import CSS there is no issues. When I run the e2e locally it works.
> test-kitchensink@0.1.0 build /private/var/folders/11/rrgwrzvj5lggl1yn4rlz3qyh0000gn/T/tmp.IKl6bVyn/test-kitchensink
> react-scripts build
Creating an optimized production build...
Compiled successfully. I have even re-forked the repo, redone each change while running e2e tests and made sure no changes broke anything. Can someone else pull this branch and trying running e2e, see if they get an error and what might be causing it? |
I tried running it locally. It failed with with the same error as on CI. The issue I can see here is I tried to use a function for test: input => !/\.modules.css$/.test(input), I still got the same error, though. |
|
||
```html | ||
<div class="Button-modules__button___1o1Ru"></div>; | ||
} |
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.
This }
is wrong and the line above shouldn't have a ;
. Otherwise looks good. 👍
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.
Thanks
@@ -54,6 +54,24 @@ const extractTextPluginOptions = shouldUseRelativeAssetPaths | |||
{ publicPath: Array(cssFilename.split('/').length).join('../') } | |||
: {}; | |||
|
|||
// Options for PostCSS as we reference these options twice | |||
// Adds vendor prefixing to support IE9 and above | |||
const postCSSLoaderOptions = { |
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.
A config file might be better since we are copy pasting this on the production and on the dev version.
IMO it would be even better to merge both webpack config files and simply use variables to determine the correct environment. It would also help a lot when you're ejecting it and add/change/remove things but this is just IMHO.
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.
It was copy and pasted previously as well. For now I think leaving it rather than making another config file is best and it simplifies the code base (at cost of repetition).
The issue should be addressed in #2108 when the target browsers is pulled out into the package.json config.
Thanks for the feedback. Happy to discuss further.
// Adds support for CSS Modules (https://github.com/css-modules/css-modules) | ||
// using the extension .modules.css | ||
{ | ||
test: /\.modules.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.
Shouldn't it be /\.modules\.css$/ (missing a backslash) ?
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.
Thats correct. Was working because it was just allowing [anything]css. Thanks!
5516ef1
to
eab529b
Compare
Thanks @abdulsattar you are quite right. I simplified the regex but just adding an exclude. Now for the regular css loader it simply has a test for all css files All the end to end tests are now passing, and ran against my production codebase and works great as well. |
@gaearon now passing all tests, tested in production environment, and reviewed by a few people. With 18 thumbs up in the issue, and another 3 here, and no dissenting comments. How do you feel and merging / providing feedback? |
I use Bootstrap 4 SCSS files in my project. Will it be possible to use CSS Modules with SASS? |
@cr101 - this implementation of CSS modules will have no affect on sass files. See the readme for more info. |
@ro-savage that's great news—I had thought that Sass support would conflict with this. Sass + CSS Modules has been my preferred set up for a while now, and I think this now represents a pretty good "blessed" version of CSS Modules +@gaearon |
Is it possible to create a custom build of Bootstrap 4 by composing variables and @importing the css modules that I need into a new
Then is it possible to import global and local scoped css like below?
|
I think so, but your In general, bootstrap is tough to make work in a local fashion. In fact, even having it globally on the page makes things more difficult, since you have to work against their rules' specificity. Generally, if you have to use bootstrap, leave it global and keep your own components local. If you ever want to apply a bootstrap class to your own component, you can do: .my-component {
composes: dropdown from global;
/* your own css here */
} This issue probably isn't the place for this discussion though, so if you have further questions maybe open an issue at https://github.com/css-modules/css-modules/issues |
I recall there was a conversation in Twitter about the naming but I can't find the link. Maybe it's better to use |
I am not on twitter, so missed the conversation. For anyone interested its here: https://twitter.com/dan_abramov/status/865895057802571776 Happy to change to I deliberately avoided something like @geelen & @markdalgleish have any thoughts? If CRA starts using it, you might find it becomes a defacto standard for CSS Modules. |
Let's use |
eab529b
to
e59831a
Compare
Updated to now use Is this a breaking change because someone might currently import a css file called |
Yes. |
describe('css modules inclusion', () => { | ||
it('renders without crashing', () => { | ||
const div = document.createElement('div'); | ||
ReactDOM.render(<CssModulesInclusion />, div); |
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.
How should CSS modules be interpreted by Jest? I would think that http://facebook.github.io/jest/docs/webpack.html#mocking-css-modules is a good idea.
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've added in the recommend identity-obj-proxy
method in the jest config. Still need to test properly.
import styles from './assets/style.module.css'; | ||
|
||
export default () => ( | ||
<p className={styles.cssModuleInclusion}>We love useless text.</p> |
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.
We don't have an integration test demonstrating that the class name actually gets applied. Can you please add one in the integration
folder?
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 attempted to add e2e but got stuck at const doc = await initDOM('css-modules-inclusion');
.
I must be missing something fundamental because I could not get to load the correct component or match in the switch statement
Even when I updated the regular css-inclusions import statement to bring in the CSS Module component, it was still just loading the regular css component.
There must be some other way the components are being loaded for the e2e tests outside of integration/webpack.test.js
It feels a bit weird to see |
|
75f8e7d
to
7e4657b
Compare
38a7a14
to
6ca7052
Compare
@Timer - Did you want me to bring it up to date? Or are you happy doing it? |
It should merge cleanly in CLI, we just keep rebasing next -- that's why things get funky. You can feel free if you'd like, or I'll get to it when we're ready to go with this PR. |
@ro-savage looks like its happening! just wanted to thank you for your patience on bringing this feature in. 🥂 |
6ca7052
to
8d52f0d
Compare
Let's see how CI goes. 😄 |
Thanks for all of your patience and continued involvement, @ro-savage! |
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.
Minor nits
@@ -30,6 +30,20 @@ const publicUrl = ''; | |||
// Get environment variables to inject into our app. | |||
const env = getClientEnvironment(publicUrl); | |||
|
|||
// Options for PostCSS as we reference these options twice | |||
// Adds vendor prefixing to support IE9 and above |
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.
Comment outdated (since we don't use a particular config anymore)
@@ -513,6 +514,51 @@ In development, expressing dependencies this way allows your styles to be reload | |||
|
|||
If you are concerned about using Webpack-specific semantics, you can put all your CSS right into `src/index.css`. It would still be imported from `src/index.js`, but you could always remove that import if you later migrate to a different build tool. | |||
|
|||
## Adding a CSS Modules stylesheet | |||
|
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.
We should comment this section out before landing as it would be too confusing.
Addressed nits in a follow-up. Thanks again, all! |
minimize: true, | ||
sourceMap: shouldUseSourceMap, | ||
modules: true, | ||
localIdentName: '[path]__[name]___[local]', |
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.
Should the production build expose the app architecture?
Maybe use this property only when shouldUseSourceMap
is true
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.
Personally I always go for '[name]-[local]'
. It's very readable and also deterministic, therefore it works well with e2e tests and error reporting tools. The only constraint is that you shouldn't have components with identical names, which for me is something I do anyway. Maybe other people have a different opinion here though as this is can be potential error source.
In regards to @klzns comment, I'd also appreciate not exposing the file path – at least not when shouldUseSourceMap
is false
.
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.
Why does it need to be deterministic and targetable? kind of kills the whole module thing. Why not recommend people to add additional static class if it needs to be targetable? Im upp for hashes.
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.
Why does it need to be deterministic and targetable? kind of kills the whole module thing. Why not recommend people to add additional static class if it needs to be targetable? Im upp for hashes.
Web marketing people often freak out if they cannot attach the out-of-the-box tools like HeapAnalytics or MouseFlow to specific elements of a website/webapp by id/classname (or if the classnames they attach to are changing with each release).
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.
That's another example. Using a hash is be fine by me, and the mentioned use cases can still be addressed with static classes or e.g. data attributes. It's just a personal preference I thought I'll mention in case somebody else finds this interesting.
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.
The only constraint is that you shouldn't have components with identical names, which for me is something I do anyway.
This sounds like a very artificial constraint. I expect that people will trip over this if it's not enforced.
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.
Why not recommend people to add additional static class if it needs to be targetable?
Great point @andriijas. I agree that we should recommend people decouple styling classes from identifiers for testing or the tools like the ones @sompylasar mentioned. The fact that we used styling classes for so many years as hooks for these tools wasn't because they were actually related concerns, it was just all we had.
Make it easy to do the right thing / hard to do the wrong thing.
For any further discussion please create a new issue with your concerns. It is getting hard to follow new threads in this PR. |
Support for CSS Modules has been released in the first v2 alpha version. See #3815 for more details! |
* Add css modules with [name].modules.css file convention * Add e2e for CSS Modules * Updated based on feedback * Change css modules class name to be deterministic and fix licences * Update css modules class naming convention
This adds support for CSS Modules using the explicit file naming convention
[name].module.css
When using css modules, class names follow a deterministic convention rather than the standard random hash with the covention
[directory]__[filename]___[classname]
.Given
src/components/Button/Button.module.css
with a class.primary {}
, the generated classname will besrc-components-Button__Button-module___primary
.This is done to allow targeting off elements via classname, causes minimal overhead with gzip-enabled, and allows a developer to find component location in the devtools.
See issue #2278 for more details.
Update - 6th Oct 2017
We are currently waiting on version 2 of Create-React-App to add css module support as it is a breaking change. The current release timeframe is unknown.
You may use react-scripts-cssmodules to add cssmodules in the meantime.
This is kept up to date with react-scripts and uses similar version numbers e.g.
1.0.140
equals1.0.14
.You can use it by running
npm uninstall react-scripts and
npm install react-scripts-cssmodules. Or in new projects
create-react-app my-app --scripts-version react-scripts-cssmodules`Specific questions or feedback.
I have moved PostCSS options into a variable but repeated the rest of the style-loader code. This meaning repeated code but in my opinion is easier to follow. However, it could be changed to use less repeated code?My regex is amateur is there anyway that the incorrect file could be captured by/\.modules.css$/
or the incorrect file excluded by/[^\.modules]\.css$/
I have been using this configuration in production for a couple months with no issues.
Images of working project
React
![screen shot 2017-05-20 at 3 02 19 pm](https://cloud.githubusercontent.com/assets/9244507/26273985/d7665d0c-3d91-11e7-9f6d-aa67a399ebd2.png)
Rendered component. Using both CSS Module stylesheet, and regular stylesheet for animation
![screen shot 2017-05-20 at 3 03 41 pm](https://cloud.githubusercontent.com/assets/9244507/26273988/df0375fe-3d91-11e7-942c-beefedd0e03a.png)
Rendered HTML
![screen shot 2017-05-20 at 3 02 43 pm](https://cloud.githubusercontent.com/assets/9244507/26273998/0b86e8fe-3d92-11e7-9b90-573b6543d225.png)
Applied styles
![screen shot 2017-05-20 at 3 02 52 pm](https://cloud.githubusercontent.com/assets/9244507/26274005/174bf04e-3d92-11e7-9ad8-91876d40ef21.png)
Todo before merge