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 order problem in async chunk #9949

Merged
merged 6 commits into from
Sep 22, 2022

Conversation

sanyuan0704
Copy link
Contributor

@sanyuan0704 sanyuan0704 commented Sep 1, 2022

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
Copy link
Member

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

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

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

IanVS commented Sep 4, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor

IanVS commented Sep 4, 2022

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
Copy link
Member

poyoho commented Sep 4, 2022

@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
Copy link
Member

patak-dev commented Sep 4, 2022

@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
Copy link
Contributor

IanVS commented Sep 4, 2022

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

patak-dev
patak-dev previously approved these changes Sep 4, 2022
@poyoho
Copy link
Member

poyoho commented Sep 4, 2022

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
Copy link
Contributor Author

sanyuan0704 commented Sep 4, 2022

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
Copy link
Member

@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
sapphi-red previously approved these changes Sep 8, 2022
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

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

@patak-dev patak-dev added this to the 3.2 milestone Sep 8, 2022
@patak-dev
Copy link
Member

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
Copy link
Member

@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

import { getColor, isBuild, page } from '~utils'

describe.runIf(isBuild)('build', () => {
test('should apply correct style', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, get it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

poyoho
poyoho previously approved these changes Sep 8, 2022
Copy link
Member

@poyoho poyoho left a comment

Choose a reason for hiding this comment

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

@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
Copy link
Member

poyoho commented Sep 8, 2022

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
Copy link

liect commented Sep 14, 2022

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

@sanyuan0704 sanyuan0704 requested review from patak-dev and sapphi-red and removed request for patak-dev and sapphi-red September 15, 2022 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants