-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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] Pre-serialize & cache styles to improve performance #43412
Changes from 37 commits
9045d66
f7d75a6
e19bcf7
61f432e
2cf0557
bce2ac0
db0bf76
c347735
0944431
05d06da
6f3dfb5
25cfd5c
1ef64d8
bd770e2
30c33ab
c3ab49e
4fb397d
a7a6ee5
96fce69
e09366c
509f086
1593d83
c317ce0
f2d05c1
4d0a5cb
bc451b8
a31a272
5602d8d
ae000ae
5a8a106
3b74469
deca34f
79fd545
2b52189
235993a
9ce6715
c1003b3
afd9442
0d220c4
080d166
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,31 +1,6 @@ | ||
import { CSSInterpolation } from '@mui/system'; | ||
import { unstable_memoTheme } from '@mui/system'; | ||
import { Theme } from '../styles/createTheme'; | ||
|
||
type ThemeStyleFunction = (props: { theme: Theme }) => CSSInterpolation; | ||
const memoTheme = unstable_memoTheme<Theme>; | ||
|
||
// We need to pass an argument as `{ theme }` for PigmentCSS, but we don't want to | ||
// allocate more objects. | ||
const arg = { theme: undefined as unknown as Theme }; | ||
|
||
/** | ||
* Memoize style function on theme. | ||
* Intended to be used in styled() calls that only need access to the theme. | ||
*/ | ||
export default function memoTheme(styleFn: ThemeStyleFunction) { | ||
let lastValue: CSSInterpolation; | ||
let lastTheme: Theme; | ||
|
||
return (props: { theme: Theme }) => { | ||
let value = lastValue; | ||
if (value === undefined || props.theme !== lastTheme) { | ||
arg.theme = props.theme; | ||
|
||
value = styleFn(arg); | ||
|
||
lastValue = value; | ||
lastTheme = props.theme; | ||
} | ||
|
||
return value; | ||
}; | ||
} | ||
export default memoTheme; | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
/* eslint-disable no-underscore-dangle */ | ||
import emStyled from '@emotion/styled'; | ||
import { serializeStyles as emSerializeStyles } from '@emotion/serialize'; | ||
|
||
export default function styled(tag, options) { | ||
const stylesFactory = emStyled(tag, options); | ||
|
@@ -27,13 +28,21 @@ export default function styled(tag, options) { | |
} | ||
|
||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
export const internal_processStyles = (tag, processor) => { | ||
export function internal_mutateStyles(tag, processor) { | ||
// Emotion attaches all the styles as `__emotion_styles`. | ||
// Ref: https://github.com/emotion-js/emotion/blob/16d971d0da229596d6bcc39d282ba9753c9ee7cf/packages/styled/src/base.js#L186 | ||
if (Array.isArray(tag.__emotion_styles)) { | ||
tag.__emotion_styles = processor(tag.__emotion_styles); | ||
} | ||
}; | ||
} | ||
|
||
// Emotion only accepts an array, but we want to avoid allocations | ||
const wrapper = []; | ||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
export function internal_serializeStyles(styles) { | ||
wrapper[0] = styles; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Emotion only accepts an array as argument, but I want to avoid creating allocations that aren't useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could add this as a comment wherever you use this pattern? To avoid the next maintainer removing this 6 months from now. |
||
return emSerializeStyles(wrapper); | ||
} | ||
|
||
export { ThemeContext, keyframes, css } from '@emotion/react'; | ||
export { default as StyledEngineProvider } from './StyledEngineProvider'; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it shouldn't even be exported #43440
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was worried about the typings in the initial PR (
memoTheme<T>
), having it here made it easy to include theMaterialTheme
, though material doesn't use much TS from what I can see. I could remove it altogether from material but it would make it less accessible to external users in case they want it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want us to move to a point where we force developers to provide their theme (so we can remove all @mui/system re-exports from @mui/material, so we need a great DX to not rely on a default theme, so this seems OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. So should I remove this file and import
unstable_memoTheme
from@mui/system
everywhere?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is beyond the scope of this PR #43440.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, I added the export from @mui/material because I wanted to use it in the lab, for the LoadingButton.