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.

Sign up for GitHub

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
+44 −17
Open

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f752032
set `before` directly without using prepend for global
siriwatknp Dec 4, 2024
459ebd8
replace prepend with insertion point and intercept insert
siriwatknp Dec 4, 2024
ee9183c
add inject first regression
siriwatknp Dec 6, 2024
2296e6c
Merge branch 'master' of https://github.com/mui/material-ui into fix/…
siriwatknp Dec 6, 2024
3c1a1db
run pnpm dedupe
siriwatknp Dec 6, 2024
ddc3ebb
run pnpm dedupe
siriwatknp Dec 6, 2024
7e79cd6
use color instead
siriwatknp Dec 6, 2024
6fed696
Update packages/mui-styled-engine/src/StyledEngineProvider/StyledEngi…
siriwatknp Dec 17, 2024
cdf055f
Merge branch 'master' of https://github.com/mui/material-ui into fix/…
siriwatknp Dec 17, 2024
ce51221
restore
siriwatknp Dec 17, 2024
81ade47
add comment about the change
siriwatknp Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 26 additions & 15 deletions packages/mui-styled-engine/src/StyledEngineProvider/StyledEngineProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,11 @@ import { StyleSheet } from '@emotion/sheet';

// We might be able to remove this when this issue is fixed:
// https://github.com/emotion-js/emotion/issues/2790
const createEmotionCache = (options) => {
const createEmotionCache = (options, CustomSheet) => {
const cache = createCache(options);

/**
* This is for client-side apps only.
* A custom sheet is required to make the GlobalStyles API work with `prepend: true`.
* This is because the [sheet](https://github.com/emotion-js/emotion/blob/main/packages/react/src/global.js#L94-L99) does not consume the options.
*/
class MyStyleSheet extends StyleSheet {
constructor(args) {
super(args);
this.prepend = cache.sheet.prepend;
}
}

// Do the same as https://github.com/emotion-js/emotion/blob/main/packages/cache/src/index.js#L238-L245
cache.sheet = new MyStyleSheet({
cache.sheet = new CustomSheet({
key: cache.key,
nonce: cache.sheet.nonce,
container: cache.sheet.container,
Expand All @@ -39,7 +27,30 @@ const createEmotionCache = (options) => {
// It allows developers to easily override MUI styles with other styling solutions, like CSS modules.
let cache;
if (typeof document === 'object') {
cache = createEmotionCache({ key: 'css', prepend: true });
let insertionPoint = document.querySelector('[name="emotion-insertion-point"]');
DiegoAndai marked this conversation as resolved.
Show resolved Hide resolved
if (!insertionPoint) {
insertionPoint = document.createElement('meta');
insertionPoint.setAttribute('name', 'emotion-insertion-point');
insertionPoint.setAttribute('content', '');
}
const head = document.querySelector('head');
if (head) {
head.prepend(insertionPoint);
}
DiegoAndai marked this conversation as resolved.
Show resolved Hide resolved
siriwatknp marked this conversation as resolved.
Show resolved Hide resolved
/**
* This is for client-side apps only.
* A custom sheet is required to make the GlobalStyles API injected above the insertion point.
* This is because the [sheet](https://github.com/emotion-js/emotion/blob/main/packages/react/src/global.js#L94-L99) does not consume the options.
*/
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?

this.before = insertionPoint;
}
return super.insert(rule, options);
}
}
cache = createEmotionCache({ key: 'css', insertionPoint }, MyStyleSheet);
}

export default function StyledEngineProvider(props) {
Expand Down
Loading
Loading