Skip to content

Commit

Permalink
refactor: getLocalIdent function (#1121)
Browse files Browse the repository at this point in the history
BREAKING CHANGE: the `getLocalIndent` option should be always `Function` and should always return `String` value
  • Loading branch information
evilebottnawi committed Jul 22, 2020
1 parent 069dbb0 commit d2f6bd2
Show file tree
Hide file tree
Showing 8 changed files with 1,261 additions and 1,396 deletions.
9 changes: 1 addition & 8 deletions src/options.json
Expand Up @@ -86,14 +86,7 @@
]
},
"getLocalIdent": {
"anyOf": [
{
"type": "boolean"
},
{
"instanceof": "Function"
}
]
"instanceof": "Function"
},
"namedExport": {
"description": "Use the named export ES modules.",
Expand Down
70 changes: 29 additions & 41 deletions src/utils.js
Expand Up @@ -46,18 +46,22 @@ function unescape(str) {
}

// eslint-disable-next-line no-control-regex
const filenameReservedRegex = /[<>:"/\\|?*\x00-\x1F]/g;
const filenameReservedRegex = /[<>:"/\\|?*]/g;

This comment has been minimized.

Copy link
@meglio

meglio Apr 21, 2021

Guys, you should strive for backward compatibility as much as you can. It took me days of debugging to find out that this little change made all libraries that depend on an older version of the generic-names package stop working with webpack. For example, at the time of writing, on 21 Apr 2021, more than a year after this commit, quite a popular plugin babel-plugin-react-css-modules is still incompatible, please see this issue.

This comment has been minimized.

Copy link
@alexander-akait

alexander-akait Apr 21, 2021

Member

In this case you can create custom function, can you open a new issue and describe problem(s) and example of problem?

// eslint-disable-next-line no-control-regex
const reControlChars = /[\u0000-\u001f\u0080-\u009f]/g;
const reRelativePath = /^\.+/;

function getLocalIdent(loaderContext, localIdentName, localName, options) {
const request = normalizePath(
path.relative(options.context, loaderContext.resourcePath)
);
function defaultGetLocalIdent(
loaderContext,
localIdentName,
localName,
options
) {
const { context, hashPrefix } = options;
const { resourcePath } = loaderContext;
const request = normalizePath(path.relative(context, resourcePath));

// eslint-disable-next-line no-param-reassign
options.content = `${options.hashPrefix + request}+${unescape(localName)}`;
options.content = `${hashPrefix + request}\x00${unescape(localName)}`;

This comment has been minimized.

Copy link
@meglio

meglio Apr 21, 2021

@alexander-akait An example of the issue it caused is described in detail here.

Sorry, my original commend was intended to be attached to this line. Citing myself here:

Guys, you should strive for backward compatibility as much as you can. It took me days of debugging to find out that this little change made all libraries that depend on an older version of the generic-names package stop working with webpack. For example, at the time of writing, on 21 Apr 2021, more than a year after this commit, quite a popular plugin babel-plugin-react-css-modules is still incompatible, please see this issue.

This comment has been minimized.

Copy link
@meglio

meglio Apr 21, 2021

A better long term solution could be to expose an API function that builds this expression:

${hashPrefix + request}\x00${unescape(localName)}

So that if plugins like babel-plugin-react-css-modules need it, they can use this API instead of relying on 3rd party plugins like generic-names

This comment has been minimized.

Copy link
@meglio

meglio Apr 21, 2021

Re using a custom function, not sure how it is supposed to help while the generation of options.content is hardcoded AND some logic precedes it. A custom function would mean to duplicate everything except that new \x00 symbol. Or am I misunderstanding your suggestion?

This comment has been minimized.

Copy link
@alexander-akait

alexander-akait Apr 21, 2021

Member

Yep, let's open an issue, I will look and provide better solution and describe it in docs

This comment has been minimized.

Copy link
@alexander-akait

alexander-akait Apr 21, 2021

Member

We use here more complex logic (for example we escape local) so some characters can be changed


// Using `[path]` placeholder outputs `/` we need escape their
// Also directories can contains invalid characters for css we need escape their too
Expand All @@ -67,10 +71,9 @@ function getLocalIdent(loaderContext, localIdentName, localName, options) {
.replace(/^((-?[0-9])|--)/, '_$1')
.replace(filenameReservedRegex, '-')
.replace(reControlChars, '-')
.replace(reRelativePath, '-')
.replace(/\./g, '-'),
{ isIdentifier: true }
).replace(/\\\[local\\\]/gi, localName);
).replace(/\\\[local\\]/gi, localName);
}

function normalizeUrl(url, isStringValue) {
Expand Down Expand Up @@ -126,7 +129,7 @@ function getModulesOptions(rawOptions, loaderContext) {
localIdentHashPrefix: '',
// eslint-disable-next-line no-undefined
localIdentRegExp: undefined,
getLocalIdent,
getLocalIdent: defaultGetLocalIdent,
namedExport: false,
exportLocalsConvention: 'asIs',
exportOnlyLocals: false,
Expand Down Expand Up @@ -224,44 +227,29 @@ function shouldUseModulesPlugins(options) {
}

function getModulesPlugins(options, loaderContext) {
const {
mode,
getLocalIdent,
localIdentName,
localIdentContext,
localIdentHashPrefix,
localIdentRegExp,
} = options.modules;

let plugins = [];

try {
plugins = [
modulesValues,
localByDefault({ mode: options.modules.mode }),
localByDefault({ mode }),
extractImports(),
modulesScope({
generateScopedName: function generateScopedName(exportName) {
let localIdent;

if (options.modules.getLocalIdent) {
localIdent = options.modules.getLocalIdent(
loaderContext,
options.modules.localIdentName,
exportName,
{
context: options.modules.localIdentContext,
hashPrefix: options.modules.localIdentHashPrefix,
regExp: options.modules.localIdentRegExp,
}
);
}

if (!localIdent) {
localIdent = getLocalIdent(
loaderContext,
options.modules.localIdentName,
exportName,
{
context: options.modules.localIdentContext,
hashPrefix: options.modules.localIdentHashPrefix,
regExp: options.modules.localIdentRegExp,
}
);
}

return localIdent;
generateScopedName(exportName) {
return getLocalIdent(loaderContext, localIdentName, exportName, {
context: localIdentContext,
hashPrefix: localIdentHashPrefix,
regExp: localIdentRegExp,
});
},
exportGlobals: options.modules.exportGlobals,
}),
Expand Down
76 changes: 38 additions & 38 deletions test/__snapshots__/esModule-option.test.js.snap
Expand Up @@ -118,10 +118,10 @@ import ___CSS_LOADER_ICSS_IMPORT_0___, * as ___CSS_LOADER_ICSS_IMPORT_0____NAMED
var ___CSS_LOADER_EXPORT___ = ___CSS_LOADER_API_IMPORT___(false);
___CSS_LOADER_EXPORT___.i(___CSS_LOADER_ICSS_IMPORT_0___, \\"\\", true);
// Module
___CSS_LOADER_EXPORT___.push([module.id, \\".lOpALu8t_iv2-GccTMbIq {\\\\n color: \\" + ___CSS_LOADER_ICSS_IMPORT_0____NAMED___[\\"vDef\\"] + \\";\\\\n}\\\\n\\", \\"\\"]);
___CSS_LOADER_EXPORT___.push([module.id, \\"._1yYSY3W2VgnkKdMmuxCIL1 {\\\\n color: \\" + ___CSS_LOADER_ICSS_IMPORT_0____NAMED___[\\"vDef\\"] + \\";\\\\n}\\\\n\\", \\"\\"]);
// Exports
export const vDef = \\"\\" + ___CSS_LOADER_ICSS_IMPORT_0____NAMED___[\\"vDef\\"] + \\"\\";
export const ghi = \\"lOpALu8t_iv2-GccTMbIq\\";
export const ghi = \\"_1yYSY3W2VgnkKdMmuxCIL1\\";
export default ___CSS_LOADER_EXPORT___;
"
Expand All @@ -137,7 +137,7 @@ Array [
],
Array [
"./es-module/named/nested/index.css",
".lOpALu8t_iv2-GccTMbIq {
"._1yYSY3W2VgnkKdMmuxCIL1 {
color: red;
}
",
Expand All @@ -155,9 +155,9 @@ exports[`"esModule" option should work with "namedExport" option: module 1`] = `
import ___CSS_LOADER_API_IMPORT___ from \\"../../../../../src/runtime/api.js\\";
var ___CSS_LOADER_EXPORT___ = ___CSS_LOADER_API_IMPORT___(false);
// Module
___CSS_LOADER_EXPORT___.push([module.id, \\".jSf5EjnYI1bvqKHBrOPz6 {\\\\n color: red;\\\\n}\\\\n\\\\n.bar {\\\\n color: red;\\\\n}\\\\n\\", \\"\\"]);
___CSS_LOADER_EXPORT___.push([module.id, \\"._3yAjbn27wJ9GVH2M-pj-hs {\\\\n color: red;\\\\n}\\\\n\\\\n.bar {\\\\n color: red;\\\\n}\\\\n\\", \\"\\"]);
// Exports
export const barBaz = \\"jSf5EjnYI1bvqKHBrOPz6\\";
export const barBaz = \\"_3yAjbn27wJ9GVH2M-pj-hs\\";
export default ___CSS_LOADER_EXPORT___;
"
Expand All @@ -167,7 +167,7 @@ exports[`"esModule" option should work with "namedExport" option: result 1`] = `
Array [
Array [
"./es-module/named/base/index.css",
".jSf5EjnYI1bvqKHBrOPz6 {
"._3yAjbn27wJ9GVH2M-pj-hs {
color: red;
}
Expand Down Expand Up @@ -286,10 +286,10 @@ var ___CSS_LOADER_EXPORT___ = ___CSS_LOADER_API_IMPORT___(false);
___CSS_LOADER_EXPORT___.i(___CSS_LOADER_AT_RULE_IMPORT_0___);
var ___CSS_LOADER_URL_REPLACEMENT_0___ = ___CSS_LOADER_GET_URL_IMPORT___(___CSS_LOADER_URL_IMPORT_0___);
// Module
___CSS_LOADER_EXPORT___.push([module.id, \\"@charset \\\\\\"UTF-8\\\\\\";\\\\n\\\\n/* Comment */\\\\n\\\\n._2uMy1nigdepJE7b3T1Visj {\\\\n color: red;\\\\n background: url(\\" + ___CSS_LOADER_URL_REPLACEMENT_0___ + \\");\\\\n}\\\\n\\", \\"\\"]);
___CSS_LOADER_EXPORT___.push([module.id, \\"@charset \\\\\\"UTF-8\\\\\\";\\\\n\\\\n/* Comment */\\\\n\\\\n.oFwPvuANP2XsfGir7HPVz {\\\\n color: red;\\\\n background: url(\\" + ___CSS_LOADER_URL_REPLACEMENT_0___ + \\");\\\\n}\\\\n\\", \\"\\"]);
// Exports
___CSS_LOADER_EXPORT___.locals = {
\\"class\\": \\"_2uMy1nigdepJE7b3T1Visj\\"
\\"class\\": \\"oFwPvuANP2XsfGir7HPVz\\"
};
export default ___CSS_LOADER_EXPORT___;
"
Expand All @@ -299,7 +299,7 @@ exports[`"esModule" option should work with a value equal to "true" and the "mod
Array [
Array [
"../../src/index.js?[ident]!./es-module/imported.css",
"._3zJoIAkhorFsUbhn0phThb {
"._2sn2s-Iv44Mnv3FmSmFVuI {
color: red;
}
",
Expand All @@ -311,7 +311,7 @@ Array [
/* Comment */
._2uMy1nigdepJE7b3T1Visj {
.oFwPvuANP2XsfGir7HPVz {
color: red;
background: url(/webpack/public/path/img.png);
}
Expand All @@ -335,10 +335,10 @@ var ___CSS_LOADER_EXPORT___ = ___CSS_LOADER_API_IMPORT___(false);
___CSS_LOADER_EXPORT___.i(___CSS_LOADER_AT_RULE_IMPORT_0___);
var ___CSS_LOADER_URL_REPLACEMENT_0___ = ___CSS_LOADER_GET_URL_IMPORT___(___CSS_LOADER_URL_IMPORT_0___);
// Module
___CSS_LOADER_EXPORT___.push([module.id, \\"@charset \\\\\\"UTF-8\\\\\\";\\\\n\\\\n/* Comment */\\\\n\\\\n._2uMy1nigdepJE7b3T1Visj {\\\\n color: red;\\\\n background: url(\\" + ___CSS_LOADER_URL_REPLACEMENT_0___ + \\");\\\\n}\\\\n\\", \\"\\"]);
___CSS_LOADER_EXPORT___.push([module.id, \\"@charset \\\\\\"UTF-8\\\\\\";\\\\n\\\\n/* Comment */\\\\n\\\\n.oFwPvuANP2XsfGir7HPVz {\\\\n color: red;\\\\n background: url(\\" + ___CSS_LOADER_URL_REPLACEMENT_0___ + \\");\\\\n}\\\\n\\", \\"\\"]);
// Exports
___CSS_LOADER_EXPORT___.locals = {
\\"class\\": \\"_2uMy1nigdepJE7b3T1Visj\\"
\\"class\\": \\"oFwPvuANP2XsfGir7HPVz\\"
};
export default ___CSS_LOADER_EXPORT___;
"
Expand All @@ -348,7 +348,7 @@ exports[`"esModule" option should work with a value equal to "true" and the "mod
Array [
Array [
"../../src/index.js?[ident]!./es-module/imported.css",
"._3zJoIAkhorFsUbhn0phThb {
"._2sn2s-Iv44Mnv3FmSmFVuI {
color: red;
}
",
Expand All @@ -360,7 +360,7 @@ Array [
/* Comment */
._2uMy1nigdepJE7b3T1Visj {
.oFwPvuANP2XsfGir7HPVz {
color: red;
background: url(/webpack/public/path/img.png);
}
Expand Down Expand Up @@ -429,14 +429,14 @@ exports[`"esModule" option should work with commonjs css-loader + commonjs style
exports[`"esModule" option should work with commonjs css-loader + commonjs style-loader: result 1`] = `
Object {
"css": Object {
"body": "uTw0Zyl5UXHez5t_lWCWm",
"footer": "_2QW_-wZzF9G9_VrEnlztAG",
"header-baz": "_24LP0msVwbV2gDQs7MsBEI",
"body": "BwpsNB5_0Pze7vq8KQOAN",
"footer": "_10gZ0Ke9A0uqq8u0Z2eRKA",
"header-baz": "_2n-RuEOd5cLXo65ILWgtue",
},
"html": "
<div class=\\"_24LP0msVwbV2gDQs7MsBEI\\">
<div class=\\"uTw0Zyl5UXHez5t_lWCWm\\">
<div class=\\"_2QW_-wZzF9G9_VrEnlztAG\\">
<div class=\\"_2n-RuEOd5cLXo65ILWgtue\\">
<div class=\\"BwpsNB5_0Pze7vq8KQOAN\\">
<div class=\\"_10gZ0Ke9A0uqq8u0Z2eRKA\\">
",
}
`;
Expand All @@ -454,14 +454,14 @@ exports[`"esModule" option should work with commonjs css-loader + esModule style
exports[`"esModule" option should work with commonjs css-loader + esModule style-loader: result 1`] = `
Object {
"css": Object {
"body": "uTw0Zyl5UXHez5t_lWCWm",
"footer": "_2QW_-wZzF9G9_VrEnlztAG",
"header-baz": "_24LP0msVwbV2gDQs7MsBEI",
"body": "BwpsNB5_0Pze7vq8KQOAN",
"footer": "_10gZ0Ke9A0uqq8u0Z2eRKA",
"header-baz": "_2n-RuEOd5cLXo65ILWgtue",
},
"html": "
<div class=\\"_24LP0msVwbV2gDQs7MsBEI\\">
<div class=\\"uTw0Zyl5UXHez5t_lWCWm\\">
<div class=\\"_2QW_-wZzF9G9_VrEnlztAG\\">
<div class=\\"_2n-RuEOd5cLXo65ILWgtue\\">
<div class=\\"BwpsNB5_0Pze7vq8KQOAN\\">
<div class=\\"_10gZ0Ke9A0uqq8u0Z2eRKA\\">
",
}
`;
Expand All @@ -479,14 +479,14 @@ exports[`"esModule" option should work with esModule css-loader + commonjs style
exports[`"esModule" option should work with esModule css-loader + commonjs style-loader: result 1`] = `
Object {
"css": Object {
"body": "uTw0Zyl5UXHez5t_lWCWm",
"footer": "_2QW_-wZzF9G9_VrEnlztAG",
"header-baz": "_24LP0msVwbV2gDQs7MsBEI",
"body": "BwpsNB5_0Pze7vq8KQOAN",
"footer": "_10gZ0Ke9A0uqq8u0Z2eRKA",
"header-baz": "_2n-RuEOd5cLXo65ILWgtue",
},
"html": "
<div class=\\"_24LP0msVwbV2gDQs7MsBEI\\">
<div class=\\"uTw0Zyl5UXHez5t_lWCWm\\">
<div class=\\"_2QW_-wZzF9G9_VrEnlztAG\\">
<div class=\\"_2n-RuEOd5cLXo65ILWgtue\\">
<div class=\\"BwpsNB5_0Pze7vq8KQOAN\\">
<div class=\\"_10gZ0Ke9A0uqq8u0Z2eRKA\\">
",
}
`;
Expand All @@ -504,14 +504,14 @@ exports[`"esModule" option should work with esModule css-loader + esModule style
exports[`"esModule" option should work with esModule css-loader + esModule style-loader: result 1`] = `
Object {
"css": Object {
"body": "uTw0Zyl5UXHez5t_lWCWm",
"footer": "_2QW_-wZzF9G9_VrEnlztAG",
"header-baz": "_24LP0msVwbV2gDQs7MsBEI",
"body": "BwpsNB5_0Pze7vq8KQOAN",
"footer": "_10gZ0Ke9A0uqq8u0Z2eRKA",
"header-baz": "_2n-RuEOd5cLXo65ILWgtue",
},
"html": "
<div class=\\"_24LP0msVwbV2gDQs7MsBEI\\">
<div class=\\"uTw0Zyl5UXHez5t_lWCWm\\">
<div class=\\"_2QW_-wZzF9G9_VrEnlztAG\\">
<div class=\\"_2n-RuEOd5cLXo65ILWgtue\\">
<div class=\\"BwpsNB5_0Pze7vq8KQOAN\\">
<div class=\\"_10gZ0Ke9A0uqq8u0Z2eRKA\\">
",
}
`;
Expand Down
4 changes: 2 additions & 2 deletions test/__snapshots__/loader.test.js.snap
Expand Up @@ -634,7 +634,7 @@ exports[`loader should work with the "modules.auto" option and the "importLoader
color: #333;
}
/* CSS modules */
._3ZewTmcU5fqs585BjdQt_0 {
._1IwxFskZk1cbjNkH0QoBiv {
overflow-x: hidden;
overflow-y: auto;
overflow: hidden auto;
Expand All @@ -651,7 +651,7 @@ exports[`loader should work with the "modules.auto" option and the "importLoader
overflow-y: auto;
overflow: hidden auto;
}
.irHrhsl3WEJVKUxOmS7dC {
._2on1B_I_5ZLmD6I3yY37jX {
color: #333;
overflow-x: hidden;
overflow-y: auto;
Expand Down

0 comments on commit d2f6bd2

Please sign in to comment.