emotion-js / emotion

👩‍🎤 CSS-in-JS library designed for high performance style composition
https://emotion.sh/
MIT License
17.5k stars 1.11k forks source link

Remove defaultProps copy #3184

Closed oliviertassinari closed 3 months ago

oliviertassinari commented 6 months ago

Current behavior:

https://github.com/emotion-js/emotion/blob/73f688103c5307c90d7544508b6d935243e4299e/packages/styled/src/base.js#L183

Expected behavior:

No copy.

Context

React 19 removed the support for defaultProps on function components, and warned against it since 18.3.0, https://react.dev/blog/2024/04/25/react-19-upgrade-guide#removed-proptypes-and-defaultprops since it was deprecated 5 years ago: https://github.com/facebook/react/pull/16210.

I think that at one point, we will want to remove this line to save bundle size and avoid confusion like in https://github.com/mui/mui-x/issues/11531 or https://github.com/react-simple-code-editor/react-simple-code-editor/pull/125#issuecomment-2204531315.

Environment information:

Andarist commented 6 months ago

We have to plan to do this in the next major version that will only be compatible with React 19 (at least that's my current line of thinking about this).

oliviertassinari commented 6 months ago

It's a detail, I would personally not rush it. I mostly wanted to have a place to document this likely future change.

oliviertassinari commented 4 months ago

The problem might be more widespread, just found the same issue with https://github.com/react-simple-code-editor.

Andarist commented 4 months ago

Is the core of the problem that you are facing that users get a warning about this from React?

oliviertassinari commented 4 months ago

@Andarist Correct, it's not 100% clear from the warning that the issue is with Emotion. Developers see "styled()" and emotion in the stack trace but I guess people ignore this, and just assume that it's not emotion since they don't see the error in other places:

Andarist commented 4 months ago

We can't quite remove this as it would be a breaking change. I think it would be fine to make this assignment conditional though and that should largely address this issue, right? The warning could still be produced but only if the wrapped function would actually have .defaultProps and at that point the problem would be with that component and not Emotion (the warning wouldn't quite hint at that but I doubt we can improve that)

oliviertassinari commented 4 months ago

I think it would be fine to make this assignment conditional though and that should largely address this issue, right?

This sounds about right.

Speaking of just having a conditional assignment and React 19, I have another fun one for you 😄: #3204.

Andarist commented 4 months ago

I tried a bunch of components - both function and class ones - and I can't seem to get the warning about this from React. What am I doing wrong? :thinking:

oliviertassinari commented 4 months ago

@Andarist I can only reproduce the issue on the server side: https://codesandbox.io/p/devbox/winter-sound-kn5qy8?file=%2Fpages%2Findex.js%3A19%2C23

SCR-20240710-bait

cc @aarongarciah in case you experienced this in another way.

aarongarciah commented 4 months ago

@oliviertassinari Same; I don't see the warning on the client side, only on the server. This is the same demo but using create-react-app and the warning is not triggered: https://codesandbox.io/p/sandbox/zealous-brook-dcj8f9?file=/src/App.js:12,2

Andarist commented 3 months ago

@oliviertassinari this repro is using .defaultProps - so it's... good that you have this warning there. It might be slightly confusing that Styled(Button) gets printed in the warning when the real issue is with Button but I'm not sure what we can do about it. To support the currently supported React versions we have to "forward" those .defaultProps onto the wrapper component.

oliviertassinari commented 3 months ago

@Andarist There are no alternatives to defaultProps with class components. React didn't deprecate it.

To support the currently supported React versions we have to "forward" those .defaultProps onto the wrapper component.

Right, yeah, I conclude that we can't do anything about this. It requires a breaking change, one that might not even be a better DX.

Andarist commented 3 months ago

Hmm, ok - I see the problem better now. In theory, we could detect what kind of a component we are wrapping and conditionally create a class component. But we need to use hooks... so actually we'd have to make an extra wrapper class component just to trick React here. This would grow the React tree which isn't the best for performance.

We could also try a patch like this:

diff --git a/packages/styled/src/base.js b/packages/styled/src/base.js
index 35641c39..c0f77336 100644
--- a/packages/styled/src/base.js
+++ b/packages/styled/src/base.js
@@ -106,8 +106,13 @@ let createStyled /*: CreateStyled */ = (
       }
     }

+    const defaultProps = tag.defaultProps
+
     const Styled /*: PrivateStyledComponent<Props> */ = withEmotionCache(
       (props, cache, ref) => {
+        if (defaultProps !== undefined) {
+          props = { ...defaultProps, ...props }
+        }
         const FinalTag = (shouldUseAs && props.as) || baseTag

         let className = ''
@@ -182,7 +187,6 @@ let createStyled /*: CreateStyled */ = (
               : baseTag.displayName || baseTag.name || 'Component'
           })`

-    Styled.defaultProps = tag.defaultProps
     Styled.__emotion_real = Styled
     Styled.__emotion_base = baseTag
     Styled.__emotion_styles = styles

But I'm pretty sure I've seen people doing things like:

const Div = styled.div`color: ${({ color }) => color};`
Div.defaultProps = { color: 'hotpink' }

So people might rely being able to write on .defaultProps after creating a styled component 😢

We could intercept writes with a setter... but then there will always be a one dev who also tries to read them...

I'd assume that class components are so rare today that this might not be a big issue in practice. There are ways around it... but ofc, they might just not be as ergonomic in the class case because you'd have to merge with yours defaults all over the place, in all methods etc

oliviertassinari commented 3 months ago

Yeah, I'm closing, I think this issue should be enough for developers who face this problem. It will give us a good direction.

Thanks for looking into it.