fix: css order problem in async chunk #9949

sanyuan0704 · 2022-09-01T09:26:57Z

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

Example: #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

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.

patak-dev · 2022-09-03T20:58:46Z

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

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:

vite/packages/vite/src/node/plugins/html.ts

Line 657 in 85fa2c8

chunk.viteMetadata.importedCss.forEach((file) => {

IanVS · 2022-09-04T03:14:02Z

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.

sanyuan0704 · 2022-09-04T03:18:56Z

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 · 2022-09-04T04:04:35Z

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 · 2022-09-04T04:13:46Z

@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.

#6301 (comment)

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

patak-dev · 2022-09-04T05:51:47Z

@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 · 2022-09-04T12:31:48Z

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 · 2022-09-04T12:42:48Z

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 · 2022-09-04T12:55:35Z

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 · 2022-09-04T13:17:48Z

@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?

sapphi-red

I agree with #9949 (comment) and the change looks good to me.

patak-dev · 2022-09-08T14:03:27Z

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 · 2022-09-08T14:30:30Z

@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

IanVS · 2022-09-08T14:53:36Z

playground/async-chunk-css-order/__tests__/async-chunk-css-order.spec.ts

+import { getColor, isBuild, page } from '~utils'
+
+describe.runIf(isBuild)('build', () => {
+  test('should apply correct style', async () => {


Should this maybe go in the css playground, as a subfolder, instead of its own playground?

The ideal would be if this could be added directly to the css playground without React. But if not, we may want to give this new playground a more generic name so we can test other things on it (it is similar to the preload that uses Vue also)

Ah yes I didn't notice this is using React. The tests in @poyoho's PRs didn't use react, I think this could be refactored to remove React in a similar way.

The ideal would be if this could be added directly to the css playground without React. But if not, we may want to give this new playground a more generic name so we can test other things on it (it is similar to the preload that uses Vue also)

Need i update the test file in the pr and remove react?

@sanyuan0704 maybe you could remove the new playground and use the one from @poyoho testing this particular bug here #9278. They are already coded for the css playground

Okay, get it.

i have optimized the test cases and remove react in them.

poyoho

@sapphi-red Thank you following of it. After @patak-dev comment and your comment I think my worries may be superfluous. This PR maybe can resolve a lot issues and the impact is not big too. Although there are still some unsolved problems, it should be a step in the right direction.

poyoho · 2022-09-08T15:13:35Z

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 · 2022-09-14T06:05:15Z

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 · 2022-09-14T10:55:11Z

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/

This reverts commit 81044bf.

sapphi-red · 2022-09-15T12:04:52Z

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 · 2022-09-15T15:11:27Z

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 ❤️.

fix: css order problem in async chunk

4df9062

patak-dev added p4-important Violate documented behavior or significantly improves performance (priority) feat: css labels Sep 3, 2022

patak-dev previously approved these changes Sep 4, 2022

View reviewed changes

sapphi-red previously approved these changes Sep 8, 2022

View reviewed changes

patak-dev added this to the 3.2 milestone Sep 8, 2022

IanVS reviewed Sep 8, 2022

View reviewed changes

poyoho previously approved these changes Sep 8, 2022

View reviewed changes

feat: add test case and remove react

85ed65d

sanyuan0704 dismissed stale reviews from poyoho, sapphi-red, and patak-dev via 85ed65d September 14, 2022 10:11

sanyuan0704 added 2 commits September 14, 2022 18:26

chore: remove useless html fragment

6b69754

chore: update pnpm-lock.yaml

81044bf

sanyuan0704 force-pushed the fix/style-order branch from f471fdf to 81044bf Compare September 14, 2022 10:42

sapphi-red added 2 commits September 15, 2022 20:56

Revert "chore: update pnpm-lock.yaml"

47ed53f

This reverts commit 81044bf.

chore: update pnpm-lock.yaml

b685218

sanyuan0704 requested review from patak-dev and sapphi-red and removed request for patak-dev and sapphi-red September 15, 2022 23:47

sapphi-red approved these changes Sep 18, 2022

View reviewed changes

liect mentioned this pull request Sep 19, 2022

结合UnoCSS使用时会有样式冲突 Tencent/tdesign-vue-next#1650

Closed

patak-dev approved these changes Sep 22, 2022

View reviewed changes

patak-dev merged commit 6c7b834 into vitejs:main Sep 22, 2022

atzcl mentioned this pull request Oct 18, 2022

fix: css insert order #9278

Closed

12 tasks

chenjiahan mentioned this pull request Oct 22, 2022

[Bug Report] 通过 Vite3 生产环境构建的产出 Toast 组件部分样式被 Popup 组件样式覆盖 youzan/vant#11049

Closed

lsdsjy mentioned this pull request Jan 11, 2023

Inconsistent order of styles when cssCodeSplit is false #11662

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: css order problem in async chunk #9949

fix: css order problem in async chunk #9949

sanyuan0704 commented Sep 1, 2022 •

edited

Loading

patak-dev commented Sep 3, 2022

IanVS commented Sep 4, 2022

sanyuan0704 commented Sep 4, 2022

IanVS commented Sep 4, 2022

poyoho commented Sep 4, 2022 •

edited

Loading

patak-dev commented Sep 4, 2022 •

edited

Loading

IanVS commented Sep 4, 2022

poyoho commented Sep 4, 2022 •

edited

Loading

sanyuan0704 commented Sep 4, 2022 •

edited

Loading

patak-dev commented Sep 4, 2022

sapphi-red left a comment

patak-dev commented Sep 8, 2022

sapphi-red commented Sep 8, 2022

IanVS Sep 8, 2022

patak-dev Sep 8, 2022

IanVS Sep 8, 2022

sanyuan0704 Sep 9, 2022

patak-dev Sep 9, 2022

sanyuan0704 Sep 9, 2022

sanyuan0704 Sep 16, 2022

poyoho left a comment

poyoho commented Sep 8, 2022

liect commented Sep 14, 2022

sanyuan0704 commented Sep 14, 2022

sapphi-red commented Sep 15, 2022

sanyuan0704 commented Sep 15, 2022

fix: css order problem in async chunk #9949

fix: css order problem in async chunk #9949

Conversation

sanyuan0704 commented Sep 1, 2022 • edited Loading

Additional context

What is the purpose of this pull request?

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

patak-dev commented Sep 3, 2022

IanVS commented Sep 4, 2022

sanyuan0704 commented Sep 4, 2022

IanVS commented Sep 4, 2022

poyoho commented Sep 4, 2022 • edited Loading

patak-dev commented Sep 4, 2022 • edited Loading

IanVS commented Sep 4, 2022

poyoho commented Sep 4, 2022 • edited Loading

sanyuan0704 commented Sep 4, 2022 • edited Loading

patak-dev commented Sep 4, 2022

sapphi-red left a comment

Choose a reason for hiding this comment

patak-dev commented Sep 8, 2022

sapphi-red commented Sep 8, 2022

IanVS Sep 8, 2022

Choose a reason for hiding this comment

patak-dev Sep 8, 2022

Choose a reason for hiding this comment

IanVS Sep 8, 2022

Choose a reason for hiding this comment

sanyuan0704 Sep 9, 2022

Choose a reason for hiding this comment

patak-dev Sep 9, 2022

Choose a reason for hiding this comment

sanyuan0704 Sep 9, 2022

Choose a reason for hiding this comment

sanyuan0704 Sep 16, 2022

Choose a reason for hiding this comment

poyoho left a comment

Choose a reason for hiding this comment

poyoho commented Sep 8, 2022

liect commented Sep 14, 2022

sanyuan0704 commented Sep 14, 2022

sapphi-red commented Sep 15, 2022

sanyuan0704 commented Sep 15, 2022

sanyuan0704 commented Sep 1, 2022 •

edited

Loading

poyoho commented Sep 4, 2022 •

edited

Loading

patak-dev commented Sep 4, 2022 •

edited

Loading

poyoho commented Sep 4, 2022 •

edited

Loading

sanyuan0704 commented Sep 4, 2022 •

edited

Loading