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

[React 19] ForwardRef props are not referentially stable, breaking downstream memoizations #31613

Open
lauri865 opened this issue Nov 22, 2024 · 6 comments
Labels

Comments

@lauri865
Copy link

lauri865 commented Nov 22, 2024

Summary

I don't know if this is a known behaviour or a bug, but something that's worth highlighting at least.

ForwardRef components are not deprecated, but they're not perfectly backwards compatible either.

The mere existance of ref prop on a ForwardRef component, even if undefined, makes the component props referentially unstable, breaking a whole host of downstream memoizations.

Reproduction:
https://codesandbox.io/p/sandbox/youthful-faraday-8m98cd?file=%2Fsrc%2FApp.tsx%3A34%2C23

React 18:
https://codesandbox.io/p/sandbox/youthful-faraday-forked-32lrwm?workspaceId=8a8eeabe-fead-479a-a993-25a9868e8015

If it's a known behaviour – please highlight this in the docs. Don't think we would have gone ahead with upgrading yet until some of our dependencies would be supporting React 19, because a whole host of packages (charting, datagrids, etc.) that are performance sensitive, suddenly had performance issues.

If not, it needs some attention, as it has pretty bad performance implications.

I believe a lot of maintainers are not dropping ForwardRef yet, because they think it's perfectly backwards compatible, and it's easier to maintain backwards compatibility for them.

@lauri865 lauri865 changed the title [React 19] Props is not referentially stable with forwardRef, breaking downstream memoizations [React 19] ForwardRef props are not referentially stable, breaking downstream memoizations Nov 22, 2024
@eps1lon
Copy link
Collaborator

eps1lon commented Nov 22, 2024

Was this working in React 18?

@lauri865
Copy link
Author

lauri865 commented Nov 22, 2024

Absolutely: https://codesandbox.io/p/sandbox/youthful-faraday-forked-32lrwm?workspaceId=8a8eeabe-fead-479a-a993-25a9868e8015

Assuming it's because of backwards compatibility: if the original props are mutated to make the props match React18, then the potential cost of doing that seems much higher than just having the ref in two places. At least developers would understand why something is different. The amount of time I had to spend right now in order to try to understand why memoizations don't work – makes me want to cry.

And again, the case in point was with a 3rd party package, so I was controlling only part of the code.

@eps1lon
Copy link
Collaborator

eps1lon commented Nov 22, 2024

breaking a whole host of downstream memoizations.

I can only think of memoizations when you pass the whole props object. What other memoizations are broken by this?

@lauri865
Copy link
Author

lauri865 commented Nov 22, 2024

breaking a whole host of downstream memoizations.

I can only think of memoizations when you pass the whole props object. What other memoizations are broken by this?

That is what we hit. But it has more implications than React.useMemo/useEffect, etc.. Say you have a downstream React.memo component, where you pass parentProps={props}. That wouldn't work. Or if it had a custom equality checker even, it would need to do more work than before, checking for deep equality rather than referential equality of the props.

@eps1lon
Copy link
Collaborator

eps1lon commented Nov 24, 2024

Minimal repro: https://codesandbox.io/p/sandbox/referentially-equal-props-in-forwardref-7hv78x?file=%2Fpackage.json

Does not reproduce without ref e.g. just <Component />

Bisected down to fa2f82a

@lauri865
Copy link
Author

lauri865 commented Dec 9, 2024

@acdlite, I understand the core rationale of the change, but shouldn't this be at minimum highlighted in docs? And aren't there performance considerations re-creating the props object like that on every render that would hurt React 19 vs. older versions? Doesn't really matter for application code, as you can easily drop the use of forwardRef, but what about library authors that need to support React ≤18 who have to stick using forwardRef?

I wish there was at least an escape hatch from this compatibility hack that library authors can decide to opt out of if they're sure that they don't spread props after assigning the ref prop. Could have even been an eslint rule probably to ensure compatibility with React 19, but now I'm not sure what's the right solution now.

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

No branches or pull requests

2 participants