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: logic for order and media queries for imports #1018

Merged
merged 1 commit into from
Dec 12, 2019

Conversation

alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Dec 12, 2019

This PR contains a:

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

Motivation / Use-Case

logic was wrong with media and for order

Breaking Changes

Should be not, but feel free to open new issue if you have problem(s)

Additional Info

No

@codecov
Copy link

codecov bot commented Dec 12, 2019

Codecov Report

Merging #1018 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1018      +/-   ##
==========================================
- Coverage   99.56%   99.56%   -0.01%     
==========================================
  Files          10       10              
  Lines         462      456       -6     
  Branches      131      129       -2     
==========================================
- Hits          460      454       -6     
  Misses          2        2
Impacted Files Coverage Δ
src/runtime/api.js 97.05% <100%> (-0.45%) ⬇️

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 cbca64d...1fb5134. Read the comment docs.

@alexander-akait alexander-akait merged commit 65450d9 into master Dec 12, 2019
@alexander-akait alexander-akait deleted the fix-order-logic-for-import-at-rules branch December 12, 2019 16:48
@jquense
Copy link
Contributor

jquense commented Jan 9, 2020

@evilebottnawi does the order change here go with the style-loader change? The removal of the dedupe logic here is the reason why we are suddenly getting ordering issues in css-modules. Can we add that back? I can send a PR

@alexander-akait
Copy link
Member Author

It is not affected on style-loader, it was a bug, look at the tests

@jquense
Copy link
Contributor

jquense commented Jan 10, 2020

I see a bug for media queries but not a test for the dedupe logic which was also removed here.

The removal of the logic to prevent modules from being included twice was a breaking change. Between this change and the style-loader change css-modules basically can't be used anymore, we've had to pin to older versions otherwise all our apps break.

@alexander-akait
Copy link
Member Author

The removal of the logic to prevent modules from being included twice was a breaking change. Between this change and the style-loader change css-modules basically can't be used anymore, we've had to pin to older versions otherwise all our apps break.

No, style-loader insert additional module due bug in algorithm inserting into DOM, look at order test for style-loader in manual, return old version and you will see what it is invalid and broken

@alexander-akait
Copy link
Member Author

alexander-akait commented Jan 10, 2020

Look at here too https://github.com/webpack-contrib/css-loader/pull/1018/files#diff-1a1fda967405ba229730a1fd3fb9e9cbR3, it was invalid and break css spec about @import

@jquense
Copy link
Contributor

jquense commented Jan 10, 2020

No, style-loader insert additional module due bug in algorithm inserting into DOM

yes and there is ALSO a breaking change here. I realize that there was a bug around css imports, but the fix also breaks css-modules and makes it unusable. css-module composes and @value cannot work like css imports, they are not in the spec and are fundamentally a different type of import. A fix to normal @import is great, but that fix shouldn't apply to icss imports

Without the deduping logic, composes and @value from other files cause styles in dependencies to be included again, so even with an old version of style-loader <1.1.0 the order for css modules is now wrong.

@jquense
Copy link
Contributor

jquense commented Jan 10, 2020

The fix, I think, is that icss import's should be treated separately from url or @import imports, they should follow the same rules as JS modules and not be included multiple times. This sort of change tho requires a runtime export change and needs coordination with style-loader and mini-extract-css-plugin...and should be probably be a major version

@alexander-akait
Copy link
Member Author

alexander-akait commented Jan 10, 2020

It is bug style-loader, here potential logic to fix it:

const identifierCountMap = {};

for (let i = 0; i < list.length; i++) {
    const item = list[i];
    const identifier = options.base ? item[0] + options.base : item[0];
    const count = identifierCountMap[identifier] || 0;

    identifierCountMap[identifier] = count + 1;

    const part = {
      css: item[1],
      media: item[2],
      sourceMap: item[3],
    };

    if (!stylesInDom[identifier]) {
      stylesInDom[identifier] = [];
    }

    if (stylesInDom[identifier][count]) {
      stylesInDom[identifier][count](part);
    } else {
      stylesInDom[identifier][count] = addStyle(part, options);
    }
}

It is solve problem with @import and solve problem with css modules. As i said before it is bug and it was be fixed, try to use mini-extract-css-plugin - all works fine. So it is bug it can't be reverted.

@jquense
Copy link
Contributor

jquense commented Jan 10, 2020

I realize there is a bug in style-loader. However, We downgraded style-loader and css-modules was still broken with this change. Since this changes what is exported it may also break mini-extract-css-plugin, unless it dedupes styles

Consider this case:

base-button.css

.btn {
  height: 30px;
}

custom-button.css

.custom-btn {
  composes: btn from './base-button.css';

  height: 60px;
}

toolbar.css

@value btn from './base-button.css';

.toolbar > btn {
  margin-left: 10px
}

page.css

.page-btn {
  composes: custom-btn from './custom-button.css';
}

.page-toolbar {
  composes: toolbar from './toolbar.css'
}

before this change PAGE.css would compile to:

exports = [
 // exports from custom button
 ['./base-button.css', ...]
 ['./custom-button.css', ...]

 // exports from toolbar
 ['./toolbar.css', ...] 

 // exports from page
 ['./page.css', ...]
]

NOW it looks like:

exports = [
 // exports from custom button
 ['./base-button.css', ...]
 ['./custom-button.css', ...]

 // exports from toolbar 
 ['./base-button.css', ...] // <--- used to be not included
 ['./toolbar.css', ...] 

 // exports from page
 ['./page.css', ...]
]

Because the base-button.css exports in both custom-button and toolbar aren't being duplicated there are now OLD style-loader is adding it twice resulting in base styles coming after custom-btn.

it looks like with your loader fix this is still happen, b/c there are now 2 parts for './base-button.css' so the style will be added twice

@alexander-akait
Copy link
Member Author

alexander-akait commented Jan 10, 2020

it may also break mini-extract-css-plugin

No, just test

it looks like with your loader fix this is still happen, b/c there are now 2 parts for './base-button.css' so the style will be added twice

No

if (stylesInDom[identifier][count]) {
  stylesInDom[identifier][count](part);
}

both will be have count === 0

Please look at code and test before saying some statements

@jquense
Copy link
Contributor

jquense commented Jan 10, 2020

Please look at code and test before some statements

I did: https://codepen.io/jquense/pen/rNavVGX?editors=0012

it never updates, only inserts.

if (stylesInDom[identifier][count]) {
  stylesInDom[identifier][count](part);
}

That is never called b/c count is different, unless it's used differently

@alexander-akait
Copy link
Member Author

alexander-akait commented Jan 10, 2020

exports = [
 // exports from custom button
 ['./base-button.css', ...]
 ['./custom-button.css', ...]

 // exports from toolbar
 ['./toolbar.css', ...] 

 // exports from page
 ['./page.css', ...]
]

Are you sure what you have this using your example? Because i have other structure with other module ids and it is allow to dedupe modules.

Do you try mini-css-extract-plugin?

@jquense
Copy link
Contributor

jquense commented Jan 10, 2020

I definitely have it in our app, but that's not minimal. I'll try and turn this into a runnable repro in a few. But intuitively if every css file re-exports it's deps you'd get duplicates naturally. I'll see if I can write a test case

@alexander-akait
Copy link
Member Author

alexander-akait commented Jan 10, 2020

@jquense webpack by default dedupe modules with same id, so mini-css-extract-plugin works fine, the problem only in style-loader because he is do not dedupe same modules

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.

2 participants