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

Rename internal forwardedRef usage #1553

Merged
merged 2 commits into from
Mar 24, 2020

Conversation

dtschust
Copy link
Contributor

This addresses issue #1552, where if a parent passes in a forwardedRef prop to a connected component (with default options), the connected child will not receive this prop. This change has the same issue with the prop reactReduxForwardedRef, but forwardedRef is a prop name commonly used and provided in React's documentation, so reactReduxForwardedRef is much less likely to be used elsewhere.

I'm happy to choose a name other than reactReduxForwardedRef if there are any suggestions, I went with something that would hopefully be clear that it is for internal use only and would be unlikely to be used by any other codebase.

A bit of context for why I'm proposing this change, if it helps. I migrated a large codebase from react-redux v5 to v7 recently. This codebase had a lot of instances of refs being passed down as a forwardedRef prop (per the suggestion in React's documentation). Many of our components are connected components in order to access feature flags. The upgrade broke all cases where a forwardedRef prop was passed through a connected component. I fixed this in the codebase by renaming all of these props refToForward and making a lint rule to forbid the use of forwardedRef. This was a little silly, but was a high confidence way to resolve the issue. I believe connect swallowing forwardedRef props is a very minor pain point and can cause confusion for developers, and since the fix isn't too involved I'm of the opinion it's worth doing.

It's your all's library though, and I understand if you don't want to merge this. Thanks for all the you do!

… namespace collision of a parent component passing in a prop called forwardedRef
@netlify
Copy link

netlify bot commented Mar 24, 2020

Deploy preview for react-redux-docs ready!

Built with commit 32630ba

https://deploy-preview-1553--react-redux-docs.netlify.com

@timdorr
Copy link
Member

timdorr commented Mar 24, 2020

Thanks!

@timdorr timdorr merged commit 172b233 into reduxjs:master Mar 24, 2020
@markerikson
Copy link
Contributor

Chen-jj added a commit to NervJS/taro that referenced this pull request Jul 27, 2020
Chen-jj added a commit to NervJS/taro that referenced this pull request Jul 28, 2020
albertodev7 pushed a commit to albertodev7/react-redux that referenced this pull request Dec 8, 2022
* Rename internal forwardedRef usage to reactReduxForwardedRef to limit namespace collision of a parent component passing in a prop called forwardedRef

* Formatting changes

Co-authored-by: Drew Schuster <dschuster@slack-corp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants