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

Update CSS Modules localIndetName #4192

Merged
merged 4 commits into from
Apr 13, 2018

Conversation

ro-savage
Copy link
Contributor

This PR updates the classname naming of CSS Modules as per the discussion in PR #3965.

Examples
MyFolder/MyComponent.module.css and class MyClass the output will be MyComponent.module_MyClass__[hash]

MyFolder/index.module.css and class MyClass the output will be MyFolder_MyClass__[hash]

The E2E tests have been updated, but I was unable to run them locally to ensure the tests were working correctly.

The results have been manually tested.

@ro-savage
Copy link
Contributor Author

CI is failed because it doesn't correctly copy/use the development version of react-dev-utils and therefore can't find react-dev-utils/getCSSModuleLocalIdent.

Also failing due to linting issues that already existed in the repo.

@Timer Timer added this to the 2.0.0 milestone Mar 21, 2018
@andriijas andriijas mentioned this pull request Mar 22, 2018
@Fabianopb
Copy link
Contributor

Hi! What's the status on this one? This would help to push #4195 forward!

@ro-savage
Copy link
Contributor Author

As far as I am aware, it's ready to merge.

@sqal
Copy link

sqal commented Apr 6, 2018

I have one question about these changes. Why do we need such a long class names in production mode? If i am not mistaken, this will always generate classes like MyFolder_MyClass__[hash] . Can we leave only hash in production for smaller file size?

@ro-savage
Copy link
Contributor Author

ro-savage commented Apr 6, 2018

@sqal - Please read #3965 if you would like the history. This has been discussed and decided on and won't be changed.

Because of gzip, the size difference even for large project is tiny. The benefits of the small file increase for targeting, testing, analytics, readability etc was decided to be worth it.

Example data, for a project with 133 classNames

File [path][name]_[local] [hash:base64:5] [name]_[local]__[hash:base64:5]
JS 109.56 KB (0 B) 109.33 KB (-241 B) 109.99 KB (+443 B)
CSS 9.99 KB (0 B) 9.5 KB (-501 B) 10.41 KB (+424 B)

@gaearon
Copy link
Contributor

gaearon commented Apr 13, 2018

Does this need a rebase? Looks like there's unrelated lint issues.

@Timer Timer force-pushed the cssmodules-localIdentName branch from 73e7588 to b409ee1 Compare April 13, 2018 18:43
@Timer Timer merged commit ae2cf07 into facebook:next Apr 13, 2018
@ro-savage
Copy link
Contributor Author

Thanks @Timer !

@Timer
Copy link
Contributor

Timer commented Apr 15, 2018

No, thank you for getting this together!

kellyrmilligan added a commit to kellyrmilligan/create-react-app that referenced this pull request May 2, 2018
* upstream/next: (35 commits)
  Update envinfo and issue template (facebook#4375)
  Update sass-loader to 7.0.1 (facebook#4376)
  Support package distribution tags (facebook#4350)
  fix broken css module support in prod (facebook#4361)
  Bumped jest version to 22.4.1 (facebook#4362)
  bump babel 7 to beta 46
  bump lint-staged to node 10 compatible version
  documentation: Added License to the README.md (facebook#4294)
  Bump `fsevents`. (facebook#4331)
  Fix typo in e2e-simple.sh comment (facebook#4323)
  Add Sass loader (facebook#4195)
  Fix some typos in README.md (facebook#4286)
  Added learnstorybook.com to Storybook links (facebook#4298)
  Document multiple build environments via `env-cmd` facebook#4071 (facebook#4117)
  Fixed link to CSS imports blog post
  Update CSS Modules localIndetName (facebook#4192)
  Enable loose mode for `class-properties` (facebook#4248)
  bump babel 7 beta (facebook#4253)
  Small typo fix facebook#4217
  Changelog for 1.1.4
  ...
kellyrmilligan added a commit to kellyrmilligan/create-react-app that referenced this pull request May 2, 2018
* next: (35 commits)
  Update envinfo and issue template (facebook#4375)
  Update sass-loader to 7.0.1 (facebook#4376)
  Support package distribution tags (facebook#4350)
  fix broken css module support in prod (facebook#4361)
  Bumped jest version to 22.4.1 (facebook#4362)
  bump babel 7 to beta 46
  bump lint-staged to node 10 compatible version
  documentation: Added License to the README.md (facebook#4294)
  Bump `fsevents`. (facebook#4331)
  Fix typo in e2e-simple.sh comment (facebook#4323)
  Add Sass loader (facebook#4195)
  Fix some typos in README.md (facebook#4286)
  Added learnstorybook.com to Storybook links (facebook#4298)
  Document multiple build environments via `env-cmd` facebook#4071 (facebook#4117)
  Fixed link to CSS imports blog post
  Update CSS Modules localIndetName (facebook#4192)
  Enable loose mode for `class-properties` (facebook#4248)
  bump babel 7 beta (facebook#4253)
  Small typo fix facebook#4217
  Changelog for 1.1.4
  ...
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
* Update CSS Modules localIndetName

* Add missing file to package

* Correct regex

* plz plz plz
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants