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

fix: deduplicate css module for @import #1044

Closed
wants to merge 3 commits into from

Conversation

fa93hws
Copy link

@fa93hws fa93hws commented Jan 22, 2020

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

composes is fixed in https://github.com/webpack-contrib/css-loader/pull/1040/files#diff-2b4ca49d4bb0a774c4d4c1672d7aa781R329 but @import isn't.
The use-case can be refereed to the added test case.

"source" which imports "black" which composes "color" and;
"source" which imports "swhite" which composes "color".
On this case, the "color" module gets duplicated.

Breaking Changes

No

Additional Info

N/A

@codecov
Copy link

codecov bot commented Jan 22, 2020

Codecov Report

Merging #1044 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1044   +/-   ##
=======================================
  Coverage   98.14%   98.14%           
=======================================
  Files          10       10           
  Lines         484      484           
  Branches      150      149    -1     
=======================================
  Hits          475      475           
  Misses          8        8           
  Partials        1        1
Impacted Files Coverage Δ
src/utils.js 98.96% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 229d36a...e9770ff. Read the comment docs.

Comment on lines -619 to -730
}
",
"",
],
Array [
"../../src/index.js?[ident]!./import/test.css",
".test {
a: a;
}
",
"",
],
Array [
"../../src/index.js?[ident]!./import/test.css",
".test {
a: a;
}
",
"",
],
Array [
"../../src/index.js?[ident]!./import/test.css",
".test {
a: a;
}
",
"",
],
Array [
"../../src/index.js?[ident]!./import/test.css",
".test {
a: a;
}
",
"",
],
Array [
"../../src/index.js?[ident]!./import/test.css",
".test {
a: a;
}
",
"",
],
Array [
"../../src/index.js?[ident]!./import/test.css",
".test {
a: a;
}
",
"",
],
Array [
"../../src/index.js?[ident]!./import/test.css",
".test {
a: a;
}
",
"",
],
Array [
"../../src/index.js?[ident]!./import/test.css",
".test {
a: a;
}
",
"",
],
Array [
"../../src/index.js?[ident]!./import/test.css",
".test {
a: a;
}
",
"screen and (orientation:landscape)",
],
Array [
"../../src/index.js?[ident]!./import/test.css",
".test {
a: a;
}
",
"screen and (orientation: landscape)",
],
Array [
"../../src/index.js?[ident]!./import/test.css",
".test {
a: a;
}
",
"screen and (orientation:landscape)",
],
Array [
"../../src/index.js?[ident]!./import/test.css",
".test {
a: a;
}
",
"screen and (orientation:landscape)",
],
Copy link
Author

Choose a reason for hiding this comment

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

They are de-duplicated.

Comment on lines 625 to +635
Array [
"./import/import.css",
"@import url(http://example.com/style.css);",
"",
true,
],
Array [
"./import/import.css",
"@import url(http://example.com/style.css);",
"",
true,
Copy link
Author

Choose a reason for hiding this comment

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

Do they need to be de-duplicated? It's the url case.
If the de-duplication is necessary in all cases, probably we should change the runtime #1040 (comment).

@alexander-akait
Copy link
Member

@import should not be deduplicate, it is invalid, please open issue with reproducible test repo

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

Successfully merging this pull request may close these issues.

None yet

2 participants