codebandits / react-app-rewire-css-modules

MIT License
69 stars 51 forks source link

localIdentName #3

Closed Ehres closed 5 years ago

Ehres commented 6 years ago

Why not use [name] in localIdentName ? ([local]___[hash:base64:5])

If all components has same root className, like .rootor .container, all component className has same name with different hash.

In general, I use [local]__[name]__[hash:base64:5] who allow to recognize className by file name.

lnhrdt commented 6 years ago

@Ehres thanks for taking the time to submit this issue.

I think you're right. I myself reuse some common class names like .title or .header across modules (as is natural when using CSS Modules). The localIdentName I chose in this tool results in ambiguous computed classes across modules in these cases. Let's change it!

I like your suggestion of adding [name] to the localIdentName but I want to make sure we're choosing the best default. The css-loader docs use this format in their examples:

[path][name]__[local]--[hash:base64:5]

Questions:

  1. I feel that [path] can help in some scenarios but is awfully verbose for most. Any opinions on choosing a default localIdentName for this tool, comparing the above to what @Ehres suggested?
  2. Should we optimize it in production builds to just the hash?

CC: @sokra (the Webpack author and main contributor to css-loader)

lnhrdt commented 6 years ago

Regarding my 2nd question, I like @andriijas's reasoning for just hashes in production. https://github.com/facebook/create-react-app/pull/2285#discussion_r162126996

andriijas commented 6 years ago

@lnhrdt i just override it in config-overrides.js because i think its way to verbose in CRA2.

  if (env === "production") {
    // Remove default polyfills
    config.entry = { main: paths.appIndexJs };

    const cssLoader = getLoader(
      config.module.rules,
      rule => String(rule.test) === String(/\.module\.css$/),
    ).loader.find(loader => loaderNameMatches(loader, "css-loader"));
    cssLoader.options.localIdentName = "myapp-[hash:base64:8]";
}
bdefore commented 5 years ago

Along a similar vein, using getLocalIdent to produce names like ComponentName:myClass:

  const path = require('path')
  const { getLoader} = require('react-app-rewired')

  const pathSeparator = path.sep
  const removeExtension = filename => filename.split('.')[0]
  const filenameFromPath = filePath => filePath.substr(filePath.lastIndexOf(pathSeparator) + 1)
  const delimiter = ':'
  const abbreviatedIdent = (resourcePath, localSelector) => {
    const componentName = removeExtension(filenameFromPath(resourcePath))
    return `${componentName}${delimiter}${localSelector}`
  }

  const rule = getLoader(
    config.module.rules,
    // rule => String(rule.test) === String(/\.module\.css$/), // did not work because test is an empty object for an unknown reason
    rule => rule.use && rule.use[1] && rule.use[1].options.modules === true
  )

  const { localIdentName, ...previousOptions } = rule.use[1].options
  rule.use[1].options = {
    ...previousOptions,
    getLocalIdent: (context, localIdentName, localName) => abbreviatedIdent(context.resourcePath, localName)
  }
lnhrdt commented 5 years ago

Closing this issue and archiving this project in favor of Create React App's native support for CSS Modules introduced in v2.