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.

Sign up for GitHub

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

CSS Modules localIdentName cleanup #3965

Closed
wants to merge 3 commits into from
Closed

CSS Modules localIdentName cleanup #3965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6b7091e
Cleanup CSS Modules localIdentName (dev)
lnhrdt Feb 3, 2018
8731e2e
Cleanup CSS Modules localIdentName (prod)
lnhrdt Feb 3, 2018
cafff9d
Update webpack integration test for CSS Modules localIdentName changes
lnhrdt Feb 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-scripts/config/webpack.config.dev.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ module.exports = {
options: {
importLoaders: 1,
modules: true,
localIdentName: '[path]__[name]___[local]',
localIdentName: '[name]-[local]',
Copy link
Contributor

Choose a reason for hiding this comment

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

This would allow clashes in names. Its unlikely, but could happen, so add it as a possibility?

Having difference between the final output and dev, can also cause friction as it makes it much harder to look at the production site and find the issue when running in dev mode because all your class names are unrelated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree I would leave it as is

},
},
{
Expand Down
2 changes: 1 addition & 1 deletion packages/react-scripts/config/webpack.config.prod.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ module.exports = {
minimize: true,
sourceMap: shouldUseSourceMap,
modules: true,
localIdentName: '[path]__[name]___[local]',
Copy link
Contributor

Choose a reason for hiding this comment

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

As per suggestions from Dan, you can try something along these lines for both dev and prod.

{
  loader: require.resolve('css-loader'),
  options: {
    importLoaders: 1,
    modules: true,
    getLocalIdent: (context, localIdentName, localName, options) => {
      // Use the filename or folder name, based on some uses the index.js / index.module.css project style
      const fileNameOrFolder = context.resourcePath.endsWith('index.module.css') ? '[folder]' : '[name]'
      // Create a hash based on a the file location and class name. Will be unique across a project, and close to globally unique.
      const hash = loaderUtils.getHashDigest(context.resourcePath + localName, 'md5', 'base64', 5)
      // Use loaderUtils to find the file or folder name
      const className = loaderUtils.interpolateName(context, fileNameOrFolder + '_' + localName + '__' + hash , options)
      // remove the .module that appears in every classname when based on the file.
      return className.replace('.module_', '_')
    },
    minimize: true,
    sourceMap: shouldUseSourceMap,
  },
},

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also "hide" this function into react-dev-utils/getCSSModuleLocalIdent so that it doesn't get duplicated in both places.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ro-savage Thank you so much. Was on my todo list to use getLocalIdent to solve the src/component/button/index.module.css or src/component/button/button.module.css class naming convention headache.

localIdentName: '[hash:base64:5]',
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still deterministic, it is just smaller and no longer useful for a human to read/look at.

Copy link
Contributor

Choose a reason for hiding this comment

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

it is just smaller and no longer useful for a human to read/look at

Yes this is exactly the point. Developers should not use those values. And it is nice that they are small.

Copy link

@AleksandrZhukov AleksandrZhukov Feb 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Developers should not use those values.

Sometimes you have to overwrite some styles but you have no access to this this component (eg component library), so the only way that you have - is to use this localIdentName, eg [class*="foo__bar__baz"]

Will be better to have ability to configure this

},
},
{
Expand Down
4 changes: 1 addition & 3 deletions packages/react-scripts/fixtures/kitchensink/integration/webpack.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,7 @@ describe('Integration', () => {

expect(
doc.getElementsByTagName('style')[0].textContent.replace(/\s/g, '')
).to.match(
/.+__style-module___cssModulesInclusion+\{background:.+;color:.+}/
);
).to.match(/\.style-module-cssModulesInclusion{background:.+;color:.+}/);
});

it('graphql files inclusion', async () => {
Expand Down