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] Fix executing server-side Emotion component as function interpolation #29290
Conversation
@@ -112,7 +112,8 @@ export default function createStyled(input = {}) { | |||
const muiStyledResolver = (styleArg, ...expressions) => { | |||
const expressionsWithDefaultTheme = expressions | |||
? expressions.map((stylesArg) => { | |||
return typeof stylesArg === 'function' | |||
// eslint-disable-next-line no-underscore-dangle | |||
return typeof stylesArg === 'function' && stylesArg.__emotion_real !== stylesArg |
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 follow here what we check for in Emotion itself:
https://github.com/emotion-js/emotion/blob/4be3391914878fd41279701bc23332b75903ec43/packages/styled/src/base.js#L30
Note that a similar fix might be required here:
https://github.com/mui-org/material-ui/blob/c6789f3b656ff6d68bae6b68522c26e0caa10841/packages/mui-system/src/createStyled.js#L168
but I didn't analyze this enough to be 100% confident that it is needed there too.
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.
Interesting, I will need to see what is the best way to do this, as the condition would be applied when using styled-components
as a styling engine too, so it could break. Maybe I will add a custom __mui_real
on both adapters for emotion
and styled-components
and change the condition to check that :)
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've tested this with styled-components
and it still works, basically because the condition is anyway always true using styled-components
. Not sure how the implementation in styled-components
is different but it works as expected. I don't like very much that we are leaking emotion implementation details here but I think this would be better than requiring each adapter to implement new API, just for the sake of fixing this issue.
It doesn't make sense to add it on line 168, because that is transforming the first arg in the styles (which is the template string itself, or a style function).
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.
My guess is that SC dont try to optimize this on the server. We dont have to skip forwardRef either but it made sense to avoid it if possible. Its probably just a microoptimazation at best though 🤷♂️
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.
Looks good, I've added a comment to explain why we need this, feel free to update it in case I haven't capture well the issue.
LGTM - ship it :) |
Thanks @Andarist 🙏 |
fixes #29078
On the server we don't use
React.forwardRef
so the created component stays as a function (when it is aforwardRef
object on the client-side). This made you to execute this function as an interpolation when the user has been using a component as a selector (so they interpolated a component into the template string):https://github.com/emotion-js/emotion/blob/4be3391914878fd41279701bc23332b75903ec43/packages/react/src/context.js#L44-L67