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.

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

Jump to bottom

feat: resolve style fields in package json #17346

Merged
merged 29 commits into from Jun 14, 2023
+450 −8
Merged

feat: resolve style fields in package json #17346

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
9e896d2
feat: style field resolution
Jun 7, 2023
21b9bb1
test: add testcases for style field resolution
Jun 7, 2023
15e1e2a
test: update snapshots
Jun 7, 2023
dcd2d51
test: update snapshots
Jun 7, 2023
4db1194
fix: dont resolve css field
Jun 7, 2023
ba6ba46
test: update tests
Jun 7, 2023
8234b14
fix: move resolution to by dependency
Jun 8, 2023
47fa1dd
test: update snapshots
Jun 8, 2023
9f27e75
test: update snapshots
Jun 8, 2023
d61b29b
test: add more tests
Jun 8, 2023
dcfa518
fix: resolver options
Jun 12, 2023
2669437
test: update snapshots
Jun 12, 2023
98d1f9c
test: add more test cases
Jun 12, 2023
426f90c
test: add contents to js file
Jun 12, 2023
f0678c1
test: add failure cases
Jun 12, 2023
537f303
test: add test cases for custom extension and data uri
Jun 13, 2023
80743de
test: update snapshots
Jun 13, 2023
57318df
test: more
alexander-akait Jun 13, 2023
805b9b6
fix: prefer relative
alexander-akait Jun 13, 2023
3c4c535
test: more
alexander-akait Jun 13, 2023
b9d3359
test: more
alexander-akait Jun 13, 2023
5f56793
test: more
alexander-akait Jun 13, 2023
e1de2dc
test: update snapshots
alexander-akait Jun 13, 2023
6cf449c
test: update
alexander-akait Jun 13, 2023
c76418a
test: update
alexander-akait Jun 13, 2023
5d1fdf7
test: more
alexander-akait Jun 13, 2023
078ce2c
test: update
alexander-akait Jun 13, 2023
0798a75
refactor: update comments
alexander-akait Jun 13, 2023
c350779
test: more
alexander-akait Jun 13, 2023
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
11 changes: 10 additions & 1 deletion lib/config/defaults.js
View file
Open in desktop
Expand Up @@ -1435,6 +1435,14 @@ const getResolveDefaults = ({ cache, context, targetProperties, mode }) => {
extensions: [...jsExtensions]
});

/** @type {function(): ResolveOptions} */
const styleDeps = () => ({
conditionNames: ["style"],
mainFields: ["style", "main", "..."],
Copy link
Member

Choose a reason for hiding this comment

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

Don't think we need the main here, because it already in list, please try to remove it from here and test it

mainFiles: ["index", "..."],
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need it here, becaue it will resolve index.css when you have @import "./directory", it was in css-loader only for legacy purposes, we don't need to put it here

Copy link
Member

Choose a reason for hiding this comment

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

So we need a test case here too

extensions: [".css", "..."]
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about [".css", "..."], because it will allow to use @import url("file.js");, it is wrong, so we need to add test cases, also please add testcases on each property here

Copy link
Member

Choose a reason for hiding this comment

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

Based on it

webpack/lib/config/defaults.js

Line 1417 in d61b29b

const jsExtensions = [".js", ".json", ".wasm"];
, there is only css should be

});

/** @type {ResolveOptions} */
const resolveOptions = {
cache,
Expand Down Expand Up @@ -1464,7 +1472,8 @@ const getResolveDefaults = ({ cache, context, targetProperties, mode }) => {
// for backward-compat: Custom Dependency
unknown: cjsDeps(),
// for backward-compat: getResolve without dependencyType
undefined: cjsDeps()
undefined: cjsDeps(),
"css-import": styleDeps()
Copy link
Member

Choose a reason for hiding this comment

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

What do you think rename it to just css, we need to replace it in dependency from css-import to css, I think it has more sense

Copy link
Member Author

Choose a reason for hiding this comment

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

CSS is already a module type. Wont that cause a collision?
https://github.com/webpack/webpack/blob/main/lib/css/CssModulesPlugin.js#L632

After renaming css-import to css in ExternalModule and CSSModule, the snapshots are failing.

Copy link
Member

Choose a reason for hiding this comment

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

Should not because, it is just a dependecy type

}
};

Expand Down
18 changes: 18 additions & 0 deletions test/Defaults.unittest.js
View file
Open in desktop
Expand Up @@ -430,6 +430,24 @@ describe("snapshots", () => {
"...",
],
},
"css-import": Object {
"conditionNames": Array [
"style",
],
"extensions": Array [
".css",
"...",
],
"mainFields": Array [
"style",
"main",
"...",
],
"mainFiles": Array [
"index",
"...",
],
},
"esm": Object {
"aliasFields": Array [
"browser",
Expand Down
15 changes: 14 additions & 1 deletion test/__snapshots__/ConfigCacheTestCases.longtest.js.snap
View file
Open in desktop

Large diffs are not rendered by default.

15 changes: 14 additions & 1 deletion test/__snapshots__/ConfigTestCases.basictest.js.snap
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions test/configCases/css/css-import/node_modules/main-field/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions test/configCases/css/css-import/node_modules/main-field/styles.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Empty file added test/configCases/css/css-import/node_modules/package-with-exports/index.cjs
View file
Open in desktop
Empty file.
Empty file added test/configCases/css/css-import/node_modules/package-with-exports/index.js
View file
Open in desktop
Empty file.
19 changes: 19 additions & 0 deletions test/configCases/css/css-import/node_modules/package-with-exports/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions test/configCases/css/css-import/node_modules/package-with-exports/style.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions test/configCases/css/css-import/node_modules/style-library/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions test/configCases/css/css-import/node_modules/style-library/styles.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions test/configCases/css/css-import/style-import.css
View file
Open in desktop
@@ -0,0 +1,3 @@
@import "style-library";
@import "main-field";
@import "package-with-exports";
1 change: 1 addition & 0 deletions test/configCases/css/css-import/style.css
View file
Open in desktop
@@ -1,3 +1,4 @@
@import "./style-import.css";
@import "print.css?foo=1";
@import url("print.css?foo=2");
@import "print.css?foo=3" layer(default);
Expand Down