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.

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

feat: add fallback if custom getLocalIdent returns null #1193

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion README.md
Expand Up @@ -848,6 +848,8 @@ Default: `undefined`

Allows to specify a function to generate the classname.
By default we use built-in function to generate a classname.
If the custom function returns `null` or `undefined`, we fall back to the
Copy link
Member

Choose a reason for hiding this comment

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

fallback 😄

built-in function to generate the classname.

**webpack.config.js**

Expand Down Expand Up @@ -1245,7 +1247,7 @@ module.exports = {

### Separating `Interoperable CSS`-only and `CSS Module` features

The following setup is an example of allowing `Interoperable CSS` features only (such as `:import` and `:export`) without using further `CSS Module` functionality by setting `compileType` option for all files that do not match `*.module.scss` naming convention. This is for reference as having `ICSS` features applied to all files was default `css-loader` behavior before v4.
The following setup is an example of allowing `Interoperable CSS` features only (such as `:import` and `:export`) without using further `CSS Module` functionality by setting `compileType` option for all files that do not match `*.module.scss` naming convention. This is for reference as having `ICSS` features applied to all files was default `css-loader` behavior before v4.
Meanwhile all files matching `*.module.scss` are treated as `CSS Modules` in this example.

An example case is assumed where a project requires canvas drawing variables to be synchronized with CSS - canvas drawing uses the same color (set by color name in JavaScript) as HTML background (set by class name in CSS).
Expand Down
31 changes: 26 additions & 5 deletions src/utils.js
Expand Up @@ -283,11 +283,32 @@ function getModulesPlugins(options, loaderContext) {
extractImports(),
modulesScope({
generateScopedName(exportName) {
return getLocalIdent(loaderContext, localIdentName, exportName, {
context: localIdentContext,
hashPrefix: localIdentHashPrefix,
regExp: localIdentRegExp,
});
let localIdent = getLocalIdent(
loaderContext,
localIdentName,
exportName,
{
context: localIdentContext,
hashPrefix: localIdentHashPrefix,
regExp: localIdentRegExp,
}
);

// A null/undefined value signals that we should invoke the default
// getLocalIdent method.
if (localIdent == null && getLocalIdent !== defaultGetLocalIdent) {
Copy link
Member

Choose a reason for hiding this comment

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

Why we need getLocalIdent !== defaultGetLocalIdent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the situation where there wasn't a custom getLocalIdent function defined and the default function returned null or undefined for some reason, I didn't want to run the default again. It wouldn't hurt anything to do so, but it also wouldn't help anything either! ;)

Copy link
Member

Choose a reason for hiding this comment

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

I think it is bad logic, we don't need getLocalIdent !== defaultGetLocalIdent, you can't use defaultGetLocalIdent in own code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, at line 140 in the utils.js, the getLocalIdent property defaults to defaultGetLocalIdent function. So if a user doesn't provide a function, they are implicitly using the default function. The intent for the !== defaultGetLocalIdent check is to handle the situation where the user didn't provide a getLocalIdent function and defaultGetLocalIdent returned a null or undefined.

That may never happen, as there may already be safeguards in place to keep that from happening - and even if it did run the same function twice, it probably wouldn't be a big deal - so that check can be removed.

localIdent = defaultGetLocalIdent(
loaderContext,
localIdentName,
exportName,
{
context: localIdentContext,
hashPrefix: localIdentHashPrefix,
regExp: localIdentRegExp,
}
);
}
return localIdent;
},
exportGlobals: options.modules.exportGlobals,
}),
Expand Down
43 changes: 43 additions & 0 deletions test/__snapshots__/modules-option.test.js.snap
Expand Up @@ -487,6 +487,49 @@ exports[`"modules" option issue #1063: result 1`] = `

exports[`"modules" option issue #1063: warnings 1`] = `Array []`;

exports[`"modules" option issue #1191 - fallback to default getLocalIdent: errors 1`] = `Array []`;

exports[`"modules" option issue #1191 - fallback to default getLocalIdent: module 1`] = `
"// Imports
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, \\".some-class {\\\\n color: red;\\\\n}\\\\n\\", \\"\\"]);
// Exports
___CSS_LOADER_EXPORT___.locals = {
\\"some-class\\": \\"some-class\\"
};
export default ___CSS_LOADER_EXPORT___;
"
`;

exports[`"modules" option issue #1191 - fallback to default getLocalIdent: result 1`] = `
Object {
"css1": Array [
Array [
"./modules/issue-1191/issue-1191.css",
".some-class {
color: red;
}
",
"",
],
],
"css2": Array [
Array [
"./modules/issue-1191/issue-1191-custom.css",
".custom-some-class {
color: red;
}
",
"",
],
],
}
`;

exports[`"modules" option issue #1191 - fallback to default getLocalIdent: warnings 1`] = `Array []`;

exports[`"modules" option should avoid unnecessary "require": errors 1`] = `Array []`;

exports[`"modules" option should avoid unnecessary "require": module 1`] = `
Expand Down
3 changes: 3 additions & 0 deletions test/fixtures/modules/issue-1191/issue-1191-custom.css
@@ -0,0 +1,3 @@
.some-class {
color: red;
}
3 changes: 3 additions & 0 deletions test/fixtures/modules/issue-1191/issue-1191.css
@@ -0,0 +1,3 @@
.some-class {
color: red;
}
8 changes: 8 additions & 0 deletions test/fixtures/modules/issue-1191/issue-1191.js
@@ -0,0 +1,8 @@
import css1 from './issue-1191.css';
import css2 from './issue-1191-custom.css';

const wrapper = { css1, css2 }

__export__ = wrapper;

export default wrapper;
22 changes: 22 additions & 0 deletions test/modules-option.test.js
Expand Up @@ -657,6 +657,28 @@ describe('"modules" option', () => {
expect(getErrors(stats)).toMatchSnapshot('errors');
});

it('issue #1191 - fallback to default getLocalIdent', async () => {
const compiler = getCompiler('./modules/issue-1191/issue-1191.js', {
modules: {
getLocalIdent: (ctx, localIdentName, localName) =>
ctx.resourcePath.includes('custom') ? `custom-${localName}` : null,
localIdentName: '[local]',
},
});
const stats = await compile(compiler);

expect(
getModuleSource('./modules/issue-1191/issue-1191.css', stats)
).toMatchSnapshot('module');

expect(getExecutedCode('main.bundle.js', compiler, stats)).toMatchSnapshot(
'result'
);

expect(getWarnings(stats)).toMatchSnapshot('warnings');
expect(getErrors(stats)).toMatchSnapshot('errors');
});

it('should work with the `exportGlobals` option (the `mode` option is `global`)', async () => {
const compiler = getCompiler(
'./modules/exportGlobals-global/exportGlobals.js',
Expand Down