Skip to content

Update CSS Modules localIndetName - #4192

Merged
Timer merged 4 commits into
react:nextfrom
ro-savage:cssmodules-localIdentName
Apr 13, 2018
Merged

Update CSS Modules localIndetName#4192
Timer merged 4 commits into
react:nextfrom
ro-savage:cssmodules-localIdentName

Conversation

@ro-savage

@ro-savagero-savage commented Mar 21, 2018

Copy link
Copy Markdown
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-savagero-savage mentioned this pull request Mar 21, 2018
Closed
@ro-savage

ro-savage commented Mar 21, 2018

Copy link
Copy Markdown
ContributorAuthor

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.

@TimerTimer added this to the 2.0.0 milestone Mar 21, 2018
@andriijasandriijas mentioned this pull request Mar 22, 2018
Merged
@Fabianopb

Fabianopb commented Apr 6, 2018

Copy link
Copy Markdown
Contributor

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

@ro-savage

ro-savage commented Apr 6, 2018

Copy link
Copy Markdown
ContributorAuthor

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

@sqal

sqal commented Apr 6, 2018•
edited
Loading

Copy link
Copy Markdown

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

ro-savage commented Apr 6, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@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]
JS109.56 KB (0 B)109.33 KB (-241 B)109.99 KB (+443 B)
CSS9.99 KB (0 B)9.5 KB (-501 B)10.41 KB (+424 B)

@gaearon

gaearon commented Apr 13, 2018

Copy link
Copy Markdown
Contributor

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

@Timer
Timerforce-pushed the cssmodules-localIdentName branch from 73e7588 to b409ee1CompareApril 13, 2018 18:43
@Timer
Timer merged commit ae2cf07 into react:nextApr 13, 2018
@ro-savage

ro-savage commented Apr 14, 2018

Copy link
Copy Markdown
ContributorAuthor

Thanks @Timer !

@Timer

Timer commented Apr 15, 2018

Copy link
Copy Markdown
Contributor

No, thank you for getting this together!

kellyrmilligan added a commit to kellyrmilligan/create-react-app that referenced this pull request May 2, 2018
@kellyrmilligan
Merge remote-tracking branch 'upstream/next' into next
b021bb4
* upstream/next: (35 commits)
Update envinfo and issue template (react#4375)
Update sass-loader to 7.0.1 (react#4376)
Support package distribution tags (react#4350)
fix broken css module support in prod (react#4361)
Bumped jest version to 22.4.1 (react#4362)
bump babel 7 to beta 46
bump lint-staged to node 10 compatible version
documentation: Added License to the README.md (react#4294)
Bump `fsevents`. (react#4331)
Fix typo in e2e-simple.sh comment (react#4323)
Add Sass loader (react#4195)
Fix some typos in README.md (react#4286)
Added learnstorybook.com to Storybook links (react#4298)
Document multiple build environments via `env-cmd` react#4071 (react#4117)
Fixed link to CSS imports blog post
Update CSS Modules localIndetName (react#4192)
Enable loose mode for `class-properties` (react#4248)
bump babel 7 beta (react#4253)
Small typo fixreact#4217
Changelog for 1.1.4
...
kellyrmilligan added a commit to kellyrmilligan/create-react-app that referenced this pull request May 2, 2018
@kellyrmilligan
Merge branch 'next' into feature/serve-app-under-homepage
af16a17
* next: (35 commits)
Update envinfo and issue template (react#4375)
Update sass-loader to 7.0.1 (react#4376)
Support package distribution tags (react#4350)
fix broken css module support in prod (react#4361)
Bumped jest version to 22.4.1 (react#4362)
bump babel 7 to beta 46
bump lint-staged to node 10 compatible version
documentation: Added License to the README.md (react#4294)
Bump `fsevents`. (react#4331)
Fix typo in e2e-simple.sh comment (react#4323)
Add Sass loader (react#4195)
Fix some typos in README.md (react#4286)
Added learnstorybook.com to Storybook links (react#4298)
Document multiple build environments via `env-cmd` react#4071 (react#4117)
Fixed link to CSS imports blog post
Update CSS Modules localIndetName (react#4192)
Enable loose mode for `class-properties` (react#4248)
bump babel 7 beta (react#4253)
Small typo fixreact#4217
Changelog for 1.1.4
...
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
@ro-savage@Timer
Update CSS Modules localIndetName (react#4192)
e7c1aab
* Update CSS Modules localIndetName
* Add missing file to package
* Correct regex
* plz plz plz
@locklockBot locked and limited conversation to collaborators Jan 20, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

CLA Signedtag: enhancement

Projects

None yet

Milestone

2.0

Development

Successfully merging this pull request may close these issues.

6 participants

@ro-savage@Fabianopb@sqal@gaearon@Timer@facebook-github-bot