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

Avoidable re-render when wrapperProps change triggers a dispatch down the tree #1354

Closed
Hypnosphi opened this issue Jul 16, 2019 · 3 comments

Comments

@Hypnosphi
Copy link
Contributor

Hypnosphi commented Jul 16, 2019

Do you want to request a feature or report a bug?

A bug

What is the current behavior?

When the wrapped component dispatches any action in cDU or uLE as a reaction to external prop change, an unnecessary re-render is made

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to a CodeSandbox (https://codesandbox.io/s/new) or RN Snack (https://snack.expo.io/) example below:

https://codesandbox.io/s/unruffled-glitter-ofxmq?fontsize=14

Open console, click "Activate"

What is the expected behavior?

Child re-renders only once after clicking the button

Which versions of React, ReactDOM/React Native, Redux, and React Redux are you using? Which browser and OS are affected by this issue? Did this work in previous versions of React Redux?

See codesandbox above

The probable reason of the bug is that lastWrapperProps.current is outdated at the moment of dispatch:
https://github.com/reduxjs/react-redux/blob/master/src/components/connectAdvanced.js#L322-L325
This leads to ownProps reference being reset to an older value:
https://github.com/reduxjs/react-redux/blob/master/src/connect/selectorFactory.js#L78

One possible workaround is to introduce a separate recentWrapperProps ref and set in immediately inside render

const recentWrapperProps = useRef()
recentWrapperProps.current = wrapperProps
@ckedar
Copy link

ckedar commented Nov 8, 2019

The issue is indeed because of stale lastWrapperProps.

At present the value of lastWrapperProps is being updated in uLE.
It should ideally be done during the render itself.

So moving the assignment lastWrapperProps.current = wrapperProps from uLE to just before return resolves the issue while passing all test cases.

Updating of other two ref values - lastChildProps and renderIsScheduled should also be moved likewise.

@markerikson
Copy link
Contributor

@ckedar : no, we should not modify refs while rendering. That should only ever be doing in the commit phase, once we know the component has finished rendering.

@markerikson
Copy link
Contributor

I'm going to close this as a WONTFIX for now, largely because we're encouraging folks to stop using connect (even though it'll be fully supported in v8).

If someone still feels strongly about this please comment, or even better file a PR to improve behavior here.

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

No branches or pull requests

3 participants