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(css): module css deep first insert #6301

Closed
wants to merge 20 commits into from

Conversation

poyoho
Copy link
Member

@poyoho poyoho commented Dec 29, 2021

Description

fix #3924

close: #6044

Additional context

deep first insert add package deps let the upper-level import delay.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@Shinigami92 Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label Dec 30, 2021
Shinigami92
Shinigami92 previously approved these changes Dec 30, 2021
patak-dev
patak-dev previously approved these changes Dec 30, 2021
@patak-dev
Copy link
Member

@IanVS would you also review this PR?

Copy link
Contributor

@IanVS IanVS left a comment

Choose a reason for hiding this comment

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

I tested out this change in my project, and while it does seem to fix the problem with css modules, I'm seeing new problems that appear to be related to importing both .css and .module.css files. So, for example, I have a Button.tsx react component, which has these imports:

import './button.css';
import styles from './Button.module.css';

Previously, the base button.css file would be loaded first, and the module css files would come afterwards in the cascade. With the changes here, it's reversed, with Button.module.css coming first. I'm sorry but I don't have time right now to dig into why that might be, or to write a new test for it.

@patak-dev
Copy link
Member

Thanks for testing it Ian!

@patak-dev patak-dev dismissed their stale review December 30, 2021 23:19

We need more tests

@IanVS
Copy link
Contributor

IanVS commented Dec 30, 2021

Thanks @poyoho for tackling this. I'll try to write a test this evening if you don't get to it first. I think you're on the right track here.

@poyoho
Copy link
Member Author

poyoho commented Dec 30, 2021

Thank @IanVS. I I thought of bug:

now is base with ./black.module.css

import styles from './black.module.css'
export function baseAsync(className, color) {
  const div = document.createElement('div')
  div.className = `${styles.black} ${className} async-modules-${color}`
  document.body.appendChild(div)
  div.textContent = `async css modules (${color}) ${
    getComputedStyle(div).color
  }`
}

Keep the current call unchanged and use xxx.module.css (unused module) as the basis in other scenarios,

import styles from './xxx.module.css'
export function baseAsync2(className, color) {
  const div = document.createElement('div')
  div.className = `${styles.black} ${className} async-modules-${color}`
  document.body.appendChild(div)
  div.textContent = `async css modules (${color}) ${
    getComputedStyle(div).color
  }`
}

call baseAsync2(styles.black,'...'), it seems to had problem😂
Because black has been inserted in the front and has a lower priority

@IanVS
Copy link
Contributor

IanVS commented Dec 31, 2021

I'm not sure I'm following, but it seems like what you described would be the desired outcome.

@poyoho
Copy link
Member Author

poyoho commented Jan 4, 2022

image

In this case, it seems that the problem cannot be solved 🤔

if package into a chunk, but it will increase the volume of CSS

@patak-dev @IanVS

@IanVS
Copy link
Contributor

IanVS commented Jan 6, 2022

I've tried to understand what's going on here, and failed. :( What do the chunks with boxes around them represent?

@poyoho
Copy link
Member Author

poyoho commented Jan 6, 2022

first used

import('blue.module.css')
import('red.module.css')

used elsewhere

import('red.module.css')
import('blue.module.css')

and then will generate

<link rel=stylesheet src=blue.module.css />
<link rel=stylesheet src=red.module.css />

so elsewhere will had error order

@IanVS
Copy link
Contributor

IanVS commented Jan 6, 2022

Hmmmm, I see. Is that something that should normally be supported? I think the same thing can essentially happen in synchronous css imports, too, right? Which is why it's important to be careful about the order in which css is imported, I guess. I think it's still worth trying to solve the original problem mentioned in #3924, and I think you're close, using the depth-first approach.

@poyoho
Copy link
Member Author

poyoho commented Jan 6, 2022

I think the solution should not be complete now. 😅
I now think that the problem should occur when importing splitechunk CSS synchronously. There must be a problem of order.
Let's take a look at the test case. The priority of class is the same. The later CSS has a higher priority. It seems that the problem of baseasync can't be solved

@IanVS
Copy link
Contributor

IanVS commented Jan 6, 2022

FWIW, I pushed up another test to #6044 that I think is reasonable to expect to work correctly, but unfortunately does not with the change in your branch. I hope it helps a little.

@IanVS
Copy link
Contributor

IanVS commented Jan 6, 2022

Oh interesting, my new test is not failing in CI here, only your test for the other edge case is. I'll have to take another look at it, I guess.

@poyoho
Copy link
Member Author

poyoho commented Jan 6, 2022

hhhh,yes. maybe depth first can resolve your test. but my tests can't be resolve

@poyoho
Copy link
Member Author

poyoho commented Jan 10, 2022

@IanVS After a few days of thinking, I don't think it is necessary to change the loading to depth first, because even depth first can't handle the situation (for example, the red->blue order is needed first, and then blue->red is needed)

In the end, I think maybe it is ideal to ensure that the results of dev and build are consistent.

Since dev should be the default loading behavior of browsers with type=module, we cannot change it to satisfy the test results.

@IanVS
Copy link
Contributor

IanVS commented Jan 10, 2022

@poyoho do you have any thoughts on how #3924 can be solved? Maybe css code-splitting is just not possible when using async imports? Or, maybe it needs to be changed so that it does not extract out common chunks in the css, so that each dynamic import is entirely self-contained?

@poyoho
Copy link
Member Author

poyoho commented Jan 10, 2022

@IanVS It may not be caused by css split code, because now this case strongly depends on the order of css, and the loading behavior of dev and build is different. My current direction is to make build have the same behavior as browser es loading.

Even I don't think there should be #3924 a way to use CSS modules, because I think it's easy to make mistakes by relying on the CSS loading order. I think we can solve #3924 this problem by making vite's dev mode and build mode have the same loading behavior.

@DanAndreasson
Copy link

Hey! Any progress on this? Running into this exact issue

@poyoho
Copy link
Member Author

poyoho commented Feb 21, 2022

It's almost finished now, but it's stuck at one point. Now the loading order should be as expected, but there is still a problem with the execution order

Why pre-load blue.module.css but execute first__vite__updateStyle is post-load red.module.css 🥲

@poyoho
Copy link
Member Author

poyoho commented Feb 21, 2022

However change the import order I do, it always load order by red -> green -> blue in dev mod.

@poyoho poyoho closed this Jul 21, 2022
@poyoho poyoho mentioned this pull request Jul 21, 2022
12 tasks
@poyoho poyoho deleted the fix/css-module-async branch July 22, 2022 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css needs test p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vite injects css assets in wrong order with dynamic import and css modules.
6 participants