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

Remove defaultProps copy #3184

Closed
oliviertassinari opened this issue May 4, 2024 · 14 comments
Closed

Remove defaultProps copy #3184

oliviertassinari opened this issue May 4, 2024 · 14 comments

Comments

@oliviertassinari
Copy link

oliviertassinari commented May 4, 2024

Current behavior:

Styled.defaultProps = tag.defaultProps

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: facebook/react#16210.

I think that at one point, we will want to remove this line to save bundle size and avoid confusion like in mui/mui-x#11531 or react-simple-code-editor/react-simple-code-editor#125 (comment).

Environment information:

  • react version: 18.3.1
  • @emotion/react version: 11.11.4
@Andarist
Copy link
Member

Andarist commented May 4, 2024

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
Copy link
Author

oliviertassinari commented May 4, 2024

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

@oliviertassinari
Copy link
Author

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

@Andarist
Copy link
Member

Andarist commented Jul 2, 2024

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

@oliviertassinari
Copy link
Author

oliviertassinari commented Jul 2, 2024

@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
Copy link
Member

Andarist commented Jul 2, 2024

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
Copy link
Author

oliviertassinari commented Jul 2, 2024

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
Copy link
Member

Andarist commented Jul 8, 2024

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? 🤔

@oliviertassinari
Copy link
Author

oliviertassinari commented Jul 9, 2024

@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
Copy link

aarongarciah commented Jul 10, 2024

@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
Copy link
Member

@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
Copy link
Author

oliviertassinari commented Jul 19, 2024

@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
Copy link
Member

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
Copy link
Author

oliviertassinari commented Jul 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants