facebook / create-react-app

Set up a modern web app by running one command.
https://create-react-app.dev
MIT License
102.69k stars 26.84k forks source link

CSS Modules – random vs deterministic class names in production #3972

Closed michaelhogg closed 6 years ago

michaelhogg commented 6 years ago

Firstly, many thanks to @ro-savage for implementing CSS Modules in #2285 (merged and ready for release in react-scripts@2.0 – see #3815) :tada:

I'm opening this issue to continue the discussion regarding random-vs-deterministic localIdentName class names in production builds.

:warning: There were some important comments from @heavi5ide and @simonrelet which apparently were forgotten about, possibly due to a force-push which made it impossible to reach the original commit containing the comments (a15df83).


For ease of reading, I'm copying the relevant comments from here:

Click :point_left:
> _[@ro-savage]_ 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 be `src-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.


and here:

Click :point_left:
> _[@ro-savage]_ I've updated the pull request to use deterministic classnames rather than random hash, as suggested by another user (sorry can't find the comment!). > > Instead of `someFileName__someClassName___hash` it becomes `directory__someFileName___someClassName`. > > This allows predictability as to what the class name will be, so that it can be targeted. > > This can can not clash because the file would have to be in the same dir + same file name + same class name. Which if it is, it is the same class anyway. It also shouldn't contribute very minimally to filesize because gzip should handle the repetition of long classnames. > > This has been how [react-scripts-cssmodules](https://www.npmjs.com/package/react-scripts-cssmodules) has been naming things for a couple months.


and here:

Click :point_left:
> _[@klzns]_ Should the production build expose the app architecture? > > _[@amannn]_ In regards to @klzns comment, I'd also appreciate not exposing the file path > > _[@andriijas]_ 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. > > _[@sompylasar]_ 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).


and here (you'll need to click the "Show outdated" link to see the comments):

Click :point_left:
> _[@recidive]_ One thing that's very useful, specially when you are using element based event tracking in your site/app, is naming classes based on component path instead of a hash so classnames don't change on every build. This can be accomplished with `localIdentName: '[path][name]__[local]'`. Making classnames less volatile is useful for automations that rely on element classnames. > > _[@ro-savage]_ Is there any security implications of showing the repos paths? > > _[@ro-savage]_ After rewriting this to hash the path, I decided to see if I could to a PR to [css-loader](https://github.com/webpack-contrib/css-loader) to add it as an option. > > Once I looked at the source, I can see they are [already generating the hash](https://github.com/webpack-contrib/css-loader/blob/22f6621a175e858bb604f5ea19f9860982305f16/lib/getLocalIdent.js#L12) based on the path. > `options.content = options.hashPrefix + request + "+" + localName;` > which translates to `relative/path/to/file+classname` > > I had never tested it before, but if you build it multiple times the classname should not change. Unless you change the file location or the classname itself. > > @recidive & @heavi5ide are you getting different classnames with each build? > > _[@heavi5ide]_ Yes I just tested this out and it seems you are correct and this shouldn't really be a problem at all. @recidive are you sure this is still an issue? It looks like the hash generation code that @ro-savage linked to in css-loader was changed "to something more reproducable" almost two years ago: webpack-contrib/css-loader@55ad466 > > _[@simonrelet]_ I'm probably missing an important thing here and I understand the need for the naming during the development, but why isn't it just `[hash:base64:*]` in production?

I think there are two key points which merit further discussion:


:one: The concerns of @klzns and @amannn regarding exposing the source code's file paths in production builds.

I share their concerns – I would feel uncomfortable seeing class names such as src-components-Button__Button-module___primary in production.  I understand the benefits of deterministic/predictable class names which don't change with each build (so that elements can be targeted by class name), but this can be achieved without exposing the source code's file paths.


:two: The fact that css-loader's [hash] token for localIdentName is actually deterministic (not random), as @heavi5ide and @simonrelet noticed.

Click for details of getLocalIdent() and interpolateName() :point_left:
CreateReactApp / ReactScripts [currently uses `css-loader` v0.28.7](https://github.com/facebook/create-react-app/blob/v1.1.0/packages/react-scripts/package.json#L33).  Looking at the source code, [the `localIdentName` template](https://github.com/webpack-contrib/css-loader/blob/v0.28.7/lib/processCss.js#L146) is [passed to `getLocalIdent()`](https://github.com/webpack-contrib/css-loader/blob/v0.28.7/lib/processCss.js#L182), which effectively generates the class name in [this way](https://github.com/webpack-contrib/css-loader/blob/v0.28.7/lib/getLocalIdent.js#L11-L14): ```js // Filepath of the CSS file, relative to the project root dir Source CSS class name options.content = path.relative(options.context, loaderContext.resourcePath) + "+" + localName; loaderUtils.interpolateName(loaderContext, localIdentName, options); ``` `interpolateName()` is [provided by Webpack's `loader-utils` library](https://github.com/webpack/loader-utils/tree/v1.1.0#interpolatename), and the description of the `[hash]` token is: > the hash of `options.content` (Buffer) (by default it's the hex digest of the md5 hash) > > `[:hash::]` optionally one can configure > * other `hashType`s, i. e. `sha1`, `md5`, `sha256`, `sha512` > * other `digestType`s, i. e. `hex`, `base26`, `base32`, `base36`, `base49`, `base52`, `base58`, `base62`, `base64` > * and `length` the length in chars The [default template for `localIdentName` is `"[hash:base64]"`](https://github.com/webpack-contrib/css-loader/blob/v0.28.7/lib/processCss.js#L146), which generates the MD5 hash (as raw binary data) and then base64-encodes it.


So [hash] is not random.  It's the digest of: <Source CSS relative filepath> "+" <Source CSS class name>


So my recommendation is to change localIdentName in packages/react-scripts/config/webpack.config.prod.js from:

localIdentName: '[path]__[name]___[local]'

to something like:

localIdentName: '[hash:base64]'

The hash will be deterministic, predictable, unique and targetable, without exposing the source code's file paths.

andriijas commented 6 years ago

I'm 100% with you. The current localident is to verbose. And more importantly if you have third parties relying on your dom you should add other target points like data-target="Foo" and not rely on how you style your components.

sompylasar commented 6 years ago

I'd like to emphasize that CSS styles being tied to HTML classes is a coincidence. Originally HTML classes were meant to describe the semantic meaning of an element in the markup regardless of the styling. CSS then targets these markup elements via one or more types of selectors (one being selector by HTML class) to add styles to them.

gaearon commented 6 years ago

At Facebook we're using something similar to CSS Modules but with a different syntax.

Our class names look like this:

I feel like this achieves a nice balance. In development it's obvious you shouldn't rely on them, and in production they're even shorter than hashes. I guess keeping a database doesn't really work in CRA context though.

For debugging, we use a special query parameters that serves "development-like" CSS class names. Again, this is something we can't easily do with webpack and static builds, but I hope this is helpful as a reference point.

klimashkin commented 6 years ago

Keep the same localIdentName, but just add https://github.com/JPeer264/node-rename-css-selectors to get gmail-like short classes for production build as I described here https://github.com/facebook/create-react-app/pull/3965#issuecomment-362887434

gaearon commented 6 years ago

I'm closing the discussion in favor of the earlier discussion in https://github.com/facebook/create-react-app/pull/3965. In particular, I propose this solution: https://github.com/facebook/create-react-app/pull/3965#pullrequestreview-93862106.