fix: css order problem in async chunk

[sanyuan0704]

In some case which use async chunk, the style of current chunk could be covered by the dependencies, cause page style to fail.

Example: https://github.com/vitejs/vite/issues/3924

Additional context

---

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

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

[patak-dev]

@poyoho @IanVS pinging you since you have been looking into this problem before. Related PRs:

• https://github.com/vitejs/vite/pull/6301
• https://github.com/vitejs/vite/pull/9278

This PR from @sanyuan0704 looks good to me. Looking at your previous PRs @poyoho I think I may be missing something here. But seeing the same scheme but in the aggregation of module preload and CSS for HTML entries, we are also doing the same order as this PR is proposing: https://github.com/vitejs/vite/blob/85fa2c89f35ecdda4ec5ed52ea50110337d98822/packages/vite/src/node/plugins/html.ts#L657

[IanVS]

Thanks for the ping, @patak-dev. I built this branch and tried it out in my app, and unfortunately it doesn't seem to fix the issue for me. I think it would be great if the tests from https://github.com/vitejs/vite/pull/9278 could be added in this PR as well, and that might illuminate the problem.

[sanyuan0704]

Thanks for the ping, @patak-dev. I built this branch and tried it out in my app, and unfortunately it doesn't seem to fix the issue for me. I think it would be great if the tests from #9278 could be added in this PR as well, and that might illuminate the problem.

I found the reproduction repo in your issue https://stackblitz.com/edit/vite-gu874q?file=main.jsx. And in this pr branch the css problem in the repo can be resolved.

[IanVS]

I don't doubt that, but there are other cases that @poyoho and I identified as he's been working extensively on this issue, which we've worked together to create test cases for. For instance, mixing css and css module imports, or one that's particularly tricky: multiple imports that share a common css import.

[poyoho]

@sanyuan0704 Thank you very much for studying this question. But the real situation is much more complicated than that test case. So I think this PR does not solve the problem of CSS order. Because this PR is the same as my first commit.

https://github.com/vitejs/vite/pull/6301#issuecomment-1008805953

If you are interested in following up on this issue, you can refer to our latest test case #9278

[patak-dev]

@IanVS @poyoho shouldnt we still apply this PR as a step in the right direction? We now have two diff orders if the preload is in HTML or it is in a dynamic import in JS. Even if we dont solve all the issues, we should at least start using the same order in both places, no?

[IanVS]

Sure I don't see why not, as long as the issue stays open. We can update the reproduction with a different test case.

[poyoho]

This change can pass some test case. However, the most fundamental reason is that the loading of dev is A -> B -> C, and the loading of prod is A -> (B + C). So If you add setTimeout to the dynamic import of the test case or change the import order in the chunk, I think the test case will not pass.

I am worried that it will affect users who used to run well with this bug.

[sanyuan0704]

This change can pass some test case. However, the most fundamental reason is that the loading of dev is A -> B -> C, and the loading of prod is A -> (B + C). So If you add setTimeout to the dynamic import of the test case or change the import order in the chunk, I think the test case will not pass.

I am worried that it will affect users who used to run well with this bug.

I think the problem solved by this pr is that the css of the asynchronous chunk itself should not be overwritten by the dependent chunk style, which should be a very reasonable scene.Because in current version, Vite will inject css link by internal preload helper function as following sequence:

chunk css link
dependencies style link

But the correct way is opposit:

dependencies style link
chunk style link

[patak-dev]

@poyoho about this,

I am worried that it will affect users who used to run well with this bug.

They should already be affected by this issue if that is the case. Right now, if you have module.js with deps on a.css and b.css (through an internal async js module) then we have:

If you do <script src="module.js"> in an entry HTML, then the order is b.css -> a.css. If you change that to a dynamic import, it is then a.css -> b.css. So depending on how we import it, we have two different orders. Looks like at least the order should be b.css -> a.css in both cases, no?

[patak-dev]

Adding this one to the 3.2 milestone to avoid potential issues. I think we should start the 3.2 beta sooner this time so we can ship this fix and others that are already piling up.

[sapphi-red]

@poyoho

the most fundamental reason is that the loading of dev is A -> B -> C, and the loading of prod is A -> (B + C).

The loading is parallel in prod, but the CSS's precedence is determined by the order of link/style tags. So it does not matter whether the loading is parallel or serial. During dev, the order of style tag is determined in the order of module evaluation. During prod, the order of link tag is determined in the order of __vitePreload. So IIUC what we need to consider is the difference between ES module evaluation order and __vitePreload order. (Since we use rollup's chunking mechanism, I think it's concatinated in the ES module evaluation order)

So If you add setTimeout to the dynamic import of the test case

Do you mean something like below?

// main.jsx
setTimeout(() => {
  import('./components/GreenButton').then(({ GreenButton }) => {
    render(<GreenButton />, document.querySelector('#green'))
  })
}, 1000)

import('./components/BlueButton').then(({ BlueButton }) => {
  render(<BlueButton />, document.querySelector('#blue'))
})

This worked with this PR. I think it's working because import('./components/BlueButton') imports Button.css -> BlueButton.css and import('./components/GreenButton') imports only GreenButton.css. (__vitePreload dedupes Button.css when importing GreenButton)

Or do you mean something like below?

// GreenButton.jsx
import React from 'react'
import { Button } from './Button'
import './GreenButton.css'

await new Promise(resolve => setTimeout(resolve, 1000))

export function GreenButton() {
  return <Button className="green">I'm a green button</Button>
}

This seems to work with this PR too. (esbuild.supported = { 'top-level-await': true } is needed) I'm not familar with top-level await module evaluation order. So there might be a case, it does not work. 🤔

change the import order in the chunk

// GreenButton.jsx
import React from 'react'
import './GreenButton.css'
import { Button } from './Button'

export function GreenButton() {
  return <Button className="green">I'm a green button</Button>
}

I think you mean the code above. Yes, this does not work with this PR. (green button should be black but it's green) The tricky point here is JS only evaluate modules once but users might not expect CSS to be evaluated only once. But I think this case can be ignored. It's very difficult to implement, I even guess it's impossible to fulfill it. IIUC Webpack's mini-css-extract-plugin also does not support this. It gives users a warning, so that might be great to have. https://github.com/webpack-contrib/mini-css-extract-plugin/blob/752b913523077d74d575c777b2bcc3239b724688/src/index.js#L1231-L1270 https://github.com/webpack-contrib/mini-css-extract-plugin/tree/master#ignoreOrder

[poyoho]

And my test case is from #9278. setTimeout case like this. I think it is impossible to guarantee the same CSS order under the current CSS loading logic. So my approach is to make the CSS loading logic of Dev and prod consistent.

And maybe we can use more runtime to fix it had the same order. But I still think it's hard to do 🤦

[liect]

This problem has bothered me for a long time. When I use UnoCSS and UI component library together, UnoCSS can correctly override UI component library styles in development environment, but not in production environment, the reason is that Vite development and production CSS are not in the same order

(This text is a machine translation, sorry, my English is not very good)

[sanyuan0704]

I'm confused that after pnpm install the nest-deps files has been influenced and i cannot commit the files in node_modules/test-pcakage-a/

[sapphi-red]

Thanks! I do not know why it didn't work in your environment, but I've pushed a commit after running pnpm i. Maybe you are using an old pnpm?

[sanyuan0704]

Thanks! I do not know why it didn't work in your environment, but I've pushed a commit after running pnpm i. Maybe you are using an old pnpm?

My local pnpm version was 7.9 which matched the vite repo.But it still cannot work.Anyway, thank you for updating the file to make the tests pass ❤️.