Skip to content

. is not replaced in filenames #980

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

Closed
rianbotha opened this issue Aug 3, 2019 · 12 comments · Fixed by #982
Closed

. is not replaced in filenames #980

rianbotha opened this issue Aug 3, 2019 · 12 comments · Fixed by #982

Comments

@rianbotha
Copy link

  • Operating System: MacOS
  • Node Version: 10.15.3
  • NPM Version: 6.4.1
  • webpack Version: 4.39.1
  • css-loader Version: 3.1.0

Expected Behavior

In 2.1.0 using this pattern for localIdentName: [name]_[local]_[hash:base64:5] the name portion would be transformed from file.module to file-module. File name is file.module.scss.

Actual Behavior

The . in file.module is no longer changed to a -.

We use babel-plugin-react-css-modules alongside css-loader. The babel plugin still transforms filenames in the same way the CSS Loader 2.* did.

Code

// CSS Loader portion of webpack config
{
  loader: 'css-loader',
  options: {
    modules: {
      localIdentName: '[name]_[local]_[hash:base64:5]',
    },
    importLoaders: 2,
    sourceMap: !production,
  },
},

How Do We Reproduce?

Use a SCSS filename with one or more . in the filename.

@alexander-akait
Copy link
Member

It is expected and breaking change for css-loader@3, babel-plugin-react-css-modules should change logic generation too

@alexander-akait
Copy link
Member

@rianbotha anyway can you create minimum reproducible test repo?

@alexander-akait
Copy link
Member

alexander-akait commented Aug 3, 2019

/cc @jquense what do you think, maybe it is not conventional for developers

@jquense
Copy link
Contributor

jquense commented Aug 3, 2019

The file.module.css or whatever css flavor (scss, less, etc) is fairly conventional at this point but I don't know that default replacing the dot is a convention...I do think that replacing invalid case characters like this is better than escaping them like we do now but that's just my preference. For my webpavk builds I do change the default getLocalIdent to replace dots with dashes

@alexander-akait
Copy link
Member

@jquense just for information, can you provide getLocalIdent source code, will be great see one of use case, maybe we can improve standard generation for better DX

@jquense
Copy link
Contributor

jquense commented Aug 4, 2019

It's pretty simple I don't escape other path characters tho since we don't use the path interpolation https://github.com/jquense/webpack-atoms/blob/master/src/index.ts#L150

@alexander-akait
Copy link
Member

Ok, let's fix it, as regression from 2.0.0

@rianbotha
Copy link
Author

rianbotha commented Aug 6, 2019

@evilebottnawi Do you still need a repo?
To get around it in the meantime I just added a custom getLocalIdent copied from the original, that just replaces . with -. A bit blunt, but solves the immediate issue for us:

const getLocalIdent = (loaderContext, localIdentName, localName, options) => {
  const filenameReservedRegex = /[<>:"/\\|?*\x00-\x1F]/g;
  const reControlChars = /[\u0000-\u001f\u0080-\u009f]/g;
  const reRelativePath = /^\.+/;

  if (!options.context) {
    options.context = loaderContext.rootContext;
  }

  const request = normalizePath(
    path.relative(options.context || '', loaderContext.resourcePath)
  );

  options.content = `${options.hashPrefix + request}+${unescape(localName)}`;

  return cssesc(
    loaderUtils
      .interpolateName(loaderContext, localIdentName, options)
      // For `[hash]` placeholder
      .replace(/^((-?[0-9])|--)/, '_$1')
      .replace(filenameReservedRegex, '-')
      .replace(reControlChars, '-')
      .replace(reRelativePath, '-')
      .replace(/\./g, '-'),
      { isIdentifier: true }
  ).replace(/\\\[local\\\]/gi, localName);
};

Might be better to add it to filenameReservedRegex?

@alexander-akait
Copy link
Member

@rianbotha no need new option, it is bug and need fix, PR welcome

@rianbotha
Copy link
Author

@evilebottnawi Thanks for the quick action.

@michaeltaranto
Copy link

FYI this was the same issue i raised here. Can also confirm that 3.2.0 fixes the issue. Thanks for getting to this 🙌

@alexander-akait
Copy link
Member

@michaeltaranto behaviour for this #966 expected when you use user function you should escape non standard characters manually, mayme we change this in next major release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants