Skip to content

fix: the auto option for inline module syntax #1274

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Mar 15, 2021
Merged

fix: the auto option for inline module syntax #1274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ba5b077
fix: the `auto` option for inline module syntax
alexander-akait Mar 15, 2021
7f5d98c
test: fix
alexander-akait Mar 15, 2021
583a9d1
fix: ident generation
alexander-akait Mar 15, 2021
fd41738
test: debug
alexander-akait Mar 15, 2021
15ebb7a
test: debug
alexander-akait Mar 15, 2021
62ef216
test: update
alexander-akait Mar 15, 2021
c44418f
refactor: code
alexander-akait Mar 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
227 changes: 115 additions & 112 deletions package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
"del": "^6.0.0",
"del-cli": "^3.0.1",
"es-check": "^5.2.3",
"eslint": "^7.21.0",
"eslint": "^7.22.0",
"eslint-config-prettier": "^8.1.0",
"eslint-plugin-import": "^2.22.1",
"file-loader": "^6.2.0",
Expand All @@ -90,7 +90,7 @@
"stylus": "^0.54.8",
"stylus-loader": "^4.3.0",
"url-loader": "^4.1.1",
"webpack": "^5.24.4"
"webpack": "^5.26.0"
},
"keywords": [
"webpack",
Expand Down
23 changes: 18 additions & 5 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,22 @@ function defaultGetLocalIdent(
localName,
options
) {
const { context, hashPrefix } = options;
const { resourcePath } = loaderContext;
const request = normalizePath(path.relative(context, resourcePath));
let relativeMatchResource = "";

// eslint-disable-next-line no-underscore-dangle
if (loaderContext._module.matchResource) {
relativeMatchResource = `${normalizePath(
// eslint-disable-next-line no-underscore-dangle
path.relative(options.context, loaderContext._module.matchResource)
)}\x00`;
}

const relativeResourcePath = normalizePath(
path.relative(options.context, loaderContext.resourcePath)
);

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

return interpolateName(loaderContext, localIdentName, options);
}
Expand Down Expand Up @@ -126,7 +136,10 @@ const moduleRegExp = /\.module(s)?\.\w+$/i;
const icssRegExp = /\.icss\.\w+$/i;

function getModulesOptions(rawOptions, loaderContext) {
const { resourcePath } = loaderContext;
const resourcePath =
// eslint-disable-next-line no-underscore-dangle
loaderContext._module.matchResource || loaderContext.resourcePath;
Copy link
Contributor

Choose a reason for hiding this comment

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

woohoo! should this also be the logic for local ident generation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, you are right, otherwise we will get the same class names.

After fix local ident generation should we merge it and release? All is fixed?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think so, seems like this covers it all. hard to know for sure until i try it out on a code base. Thanks again for helping me out here

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, here interesting case, when we have:

import three from './button.module.scss!=!./base64-loader?LmZvbyB7IGNvbG9yOiByZWQ7IH0=!./simple.js';
import four from './other.module.scss!=!./base64-loader?LmZvbyB7IGNvbG9yOiByZWQ7IH0=!./simple.js';

We run the loader only once, why? Because ES modules executed only once. So I don't think here bug. But we can solve it using:

// i.e. adding `?` with different values
import three from './button.module.scss!=!./base64-loader?LmZvbyB7IGNvbG9yOiByZWQ7IH0=!./simple.js?foo=bar';
import four from './other.module.scss!=!./base64-loader?LmZvbyB7IGNvbG9yOiByZWQ7IH0=!./simple.js?foo=baz';

Copy link
Contributor

Choose a reason for hiding this comment

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

ah interesting, so the query param acts like a cache buster. That solution seems fine to me

Copy link
Contributor

@jquense jquense Mar 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

yeah in my case it'd be easy to use a stable identifier here. This wouldn't solve the className hash generation issue tho right? I'm guessing you discovered this writing a test for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Solved, please check code

Copy link
Contributor

Choose a reason for hiding this comment

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

arg, i'm trying this out now and _module.matchResource is null locally. Going to debug a bit to see why will report back

Copy link
Member Author

Choose a reason for hiding this comment

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

If you don't have matchResource in request, it will be nullish

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah this is for requests with it in it. For some reason i am seeing the module get created twice, one with the matchResource and one without, the one without is what makes it to the css-loader. It maybe has to do with ESM, still trying to figure it out


let isIcss;

if (typeof rawOptions.modules === "undefined") {
Expand Down
47 changes: 34 additions & 13 deletions test/__snapshots__/loader.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,66 +665,66 @@ exports[`loader should work with inline module syntax: result 1`] = `
Array [
Array [
"other.modules.css!=!../../src/index.js?[ident]!./index-loader-syntax.modules.css",
"._1ZJhuRHDA53bY_Z4Qfm2b4 {
"._14oM7qkZ6dgC62uK5h4NI0 {
color: red;
}

.j3CQSVq_DdDvo0Ew74yQi {
._2NqyYysncZeLetATpl0xfW {
color: white;
}",
"",
],
Array [
"plain.scss!=!../../src/index.js?[ident]!./index-loader-syntax-sass.css",
".baz {
"button.modules.css!=!./index-loader-syntax-sass.css",
"._8aapa4kQQdSsOoRODcrwe {
width: 5px;
}",
"",
],
Array [
"other.modules.scss!=!../../src/index.js?[ident]!./index-loader-syntax-sass.modules.css",
"._1fGl5mRxLFCqIet0X3JYrB > ._2pVMnENaxk1YmKug-4EMYF {
"._2yVYVQfmDwKLZn9Qj6k_Q5 > ._3rHlnRnPEotX4nTkd82-aE {
color: red;
}",
"",
],
Array [
"other.modules.css!=!../../src/index.js?[ident]!./index-loader-syntax.modules.css",
"._1ZJhuRHDA53bY_Z4Qfm2b4 {
"._14oM7qkZ6dgC62uK5h4NI0 {
color: red;
}

.j3CQSVq_DdDvo0Ew74yQi {
._2NqyYysncZeLetATpl0xfW {
color: white;
}

.B7fdaUjwJ3YVou1v7dYEE {
._1-Aa1c8UQML46IWY0FjGXT {
from: custom;
}",
"",
],
Array [
"other.modules.css!=!../../src/index.js?[ident]!./index-loader-syntax.modules.css",
"._1ZJhuRHDA53bY_Z4Qfm2b4 {
"._14oM7qkZ6dgC62uK5h4NI0 {
color: red;
}

.j3CQSVq_DdDvo0Ew74yQi {
._2NqyYysncZeLetATpl0xfW {
color: white;
}

.B7fdaUjwJ3YVou1v7dYEE {
._1-Aa1c8UQML46IWY0FjGXT {
from: custom;
}",
"",
],
Array [
"other.modules.scss!=!../../src/index.js?[ident]!./index-loader-syntax-sass.modules.css",
"._1fGl5mRxLFCqIet0X3JYrB > ._2pVMnENaxk1YmKug-4EMYF {
"._2yVYVQfmDwKLZn9Qj6k_Q5 > ._3rHlnRnPEotX4nTkd82-aE {
color: red;
}

._1kK_VYa-N303wnLgpvL-7d {
._2kEhhupr-6tgHnCC_d4yO8 {
from: custom;
}",
"",
Expand All @@ -733,6 +733,27 @@ Array [
"./index-loader-syntax.css",
".a {
color: red;
}",
"",
],
Array [
"button.modules.css!=!./index-loader-syntax-sass.css",
"._8aapa4kQQdSsOoRODcrwe {
width: 5px;
}",
"",
],
Array [
"button.module.scss!=!./base64-loader/index.js?[ident]!./simple.js?foo=bar",
"._2hfVrRfux-FOrqimHr-fzt {
color: red;
}",
"",
],
Array [
"other.module.scss!=!./base64-loader/index.js?[ident]!./simple.js?foo=baz",
"._2jOyhMQwdpQ85V5dDLcfoG {
color: red;
Copy link
Member Author

Choose a reason for hiding this comment

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

You can see here: hash for classes are different

Copy link
Member Author

Choose a reason for hiding this comment

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

But note: you should use different matchResource, otherwise hash will be same

Copy link
Contributor

Choose a reason for hiding this comment

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

perfect!

}",
"",
],
Expand Down
3 changes: 3 additions & 0 deletions test/fixtures/base64-loader/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module.exports = function loader(content) {
return Buffer.from(this.query.slice(1), 'base64').toString('ascii');
};
10 changes: 7 additions & 3 deletions test/fixtures/index-loader-syntax.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import css from './index-loader-syntax.css';
import one from './index-loader-syntax.css';
import two from 'button.modules.css!=!./index-loader-syntax-sass.css';
// Hash should be different
import three from './button.module.scss!=!./base64-loader?LmZvbyB7IGNvbG9yOiByZWQ7IH0=!./simple.js?foo=bar';
import four from './other.module.scss!=!./base64-loader?LmZvbyB7IGNvbG9yOiByZWQ7IH0=!./simple.js?foo=baz';

__export__ = css;
__export__ = [...one, ...two, ...three, ...four];

export default css;
export default [...one, ...two, ...three, ...four];
2 changes: 1 addition & 1 deletion test/sourceMap-option.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ describe('"sourceMap" option', () => {

expect(chunkName).toBe(
webpack.version[0] === "5"
? "main.89840c70616b788038a2.bundle.js"
? "main.a531550ffe767c49e881.bundle.js"
: "main.19efc497c5c37fc5e355.bundle.js"
);
expect(
Expand Down