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

[system] set before directly without using prepend for global styles #44648

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Dec 4, 2024

closes #44597

Root cause

Each GlobalStyles create its own sheet and injected at this line:


before = this.container.firstChild

this.container.insertBefore(tag, before) // this.container = head

This cause the 2nd GlobalStyles in the app to be injected above (insertBefore) the 1st GlobalStyles.

Solution

set before to be the insertion point at the insert method of the custom sheet.

Benefit of insertion point

  • prepend (deprecated) is replaced by insertionPoint https://github.com/emotion-js/emotion/tree/main/packages/sheet#prepend.
  • prepend is unpredictable for GlobalStyles. Different order of GlobalStyles and components produce different injection order 😮 (even without <StyledEngine injectFirst>. This PR makes the order of GlobalStyles and components same as using the default cache.

Test

Added a regression test. I don't think this can be easily tested with a unit test.


@siriwatknp siriwatknp changed the title [system] set before directly without using prepend for global styles under <StyledEngine injectFirst> [system] set before directly without using prepend for global styles Dec 4, 2024
@siriwatknp siriwatknp added package: system Specific to @mui/system package: styled-engine Specific to @mui/styled-engine labels Dec 4, 2024
@mui-bot
Copy link

mui-bot commented Dec 4, 2024

Netlify deploy preview

https://deploy-preview-44648--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 81ade47

@siriwatknp siriwatknp marked this pull request as ready for review December 6, 2024 09:18
@siriwatknp siriwatknp requested a review from DiegoAndai December 6, 2024 09:18
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 9, 2024
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Hey @siriwatknp! Sorry for the delay

Have you identified with what change (PR) this broke from v5 to v6?

Should we also update this section and this warning on the docs?

*/
class MyStyleSheet extends StyleSheet {
insert(rule, options) {
if (this.key && this.key.endsWith('global')) {
Copy link
Member

Choose a reason for hiding this comment

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

Before, we weren't checking this.key.endsWith('global') to add prepend: true. Why is this required now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I was wrong before that the Emotion (with prepend) will have the same behavior if a CustomSheet is provided.

However, the behavior for the GlobalStyles is unpredictable if you order the styles differently:

<GlobalStyles />
<CssBaseline /> // this is also using GlobalStyles
<Typography />

inject the styles in different order compared to:

<Typography />
<GlobalStyles />
<CssBaseline /> // this is also using GlobalStyles

That's why we cannot rely on prepend (deprecated) but to switch to insertionPoint + custom insert API

Copy link
Member

Choose a reason for hiding this comment

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

I don't follow.

Let me rephrase my question:

  • Before this PR, we were always using prepend: true, regardless of this.key's value
  • Now, we're replacing prepend: true with insertionPoint. This makes perfect sense to me
  • But also, we're now checking for this.key.endsWith('global') to decide if we should use insertionPoint. We weren't doing that before when using prepend: true. This is what I don't understand. Why do we need to check this.key.endsWith('global') now?

Besides that, I don't understand the code examples. In both I see CssBaseline after GlobalStyles, and Typography's placement changes. I would expect Typography's placement not to have any effect on CssBaseline and GlobalStyles insertion points. Am I wrong or is the example incorrect?

@siriwatknp
Copy link
Member Author

Should we also update this section and this warning on the docs?

To be honest, I think they are separate issues, not specific to StyledEngineProvider. I believe they need a rewrite to use insertionPoint instead but it should not be in this PR.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: styled-engine Specific to @mui/styled-engine package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui] v6 changed the injection order <GlobalStyles/>
3 participants