Causes all subcomponents to re-render on any state change

[mshick]

I recently noticed that every component in my app was re-rendering with every top-level state change, even those totally unaffected. I use react-router-4, redux, reselect and Immutable.js, and React.PureComponent everywhere, and have everything pretty locked down so only the precise components I want to re-render will.

I traced the issue back to ReduxAuthWrapper and discovered that commenting out this line: https://github.com/mjrussell/redux-auth-wrapper/blob/master/src/helper/redirect.js#L50 (and fixing the subsequent issues) appears to do away with my re-renders. However, it also kills the primary function of ReduxAuthWrapper.

Before I go further with this, I wanted to see if there are any ideas about what I might be doing wrong, or any way to get around these constant re-renders. Because I use redux-form my global state is constantly changing, but I scope all my selectors to the specific domains. For some reason I can't accomplish that here, perhaps because of the way ReduxAuthWrapper itself connects to the global state?

Just for reference, here's what my implementation looks like — it's fairly verbatim from the docs and examples here:

export const userIsAuthenticated = connectedRouterRedirect({
  redirectPath: '/login',
  authenticatedSelector: state => userSelector(state).isLoggedIn,
  authenticatingSelector: state => userSelector(state).isLoggingIn,
  AuthenticatingComponent,
  wrapperDisplayName: 'UserIsAuthenticated'
});

[mjrussell]

Interesting...Thanks for reporting this. I think I may know why. The redirect location is used if the auth check fails, but we are creating a new location object here each time connect runs (any state change)

And because the authWrapper by default is passing all props down to its children here, your components nested below the HOC are getting this property, and React's should component update sees that the object are not equal (because of shallow checking) and then re-renders the tree.

Could you try using something like:

const noRedirectProp = (Component) => ({ redirect, ...otherProps }) => <Component {...otherProps} />

And then in your route/component apply it as userIsAuthenticated(noRedirectProp(MyComponent))

And let me know if that works? I removed the propMapper in 2.x but may need to add something like it back in..

[mshick]

Quick update, I believe I solved the issue for myself by implementing a simple shouldComponentUpdate function on the userAuth component.

      shouldComponentUpdate(nextProps) {
        if (this.props.isAuthenticated !== nextProps.isAuthenticated) {
          return true
        }
        if (this.props.isAuthenticating !== nextProps.isAuthenticating) {
          return true
        }
        return false
      }

See the code here: https://github.com/mshick/redux-auth-wrapper/blob/master/src/authWrapper.js#L34-L42

If this looks acceptable I can clean it up for a PR, or you could graft the function into your codebase. I've tested and all redirect functionality is still working.

And, just to put this in perspective, these unnecessary re-renders cause all kinds of trouble if you are trying to use keep a redux-form in an ephemeral state — for instance, after a form submit if you want to show a "form submitted" flash until a new significant state change happens.

[mjrussell]

Hah, similar thoughts on the SCU, any chance you can try my suggestion above to narrow it to that prop?

[mshick]

Yes, trying it out now. Get right back to you.

[mshick]

👍 Stripping the redirect prop off did the trick.

SCU strikes me as cleaner, if it doesn't cause other problems.

[mjrussell]

Let me think about this one for a little, I definitely want to push a fix soon but Im concerned that SCU at the top would prevent sub trees from reacting to prop changes properly. I think a possibly more stable alternative would be to include propMappers again, and by default omit props such as redirect from being passed to children.

[klizter]

Exact same issue here, however i did not have redirect set to anything. Applied the noRedirectProp suggested by @mjrussell and everything is now nice and responsive

[kjanoudi]

Any updates on this? Tossed me for a loop over the weekend.

[majodu]

I am also getting an issue where my container is continuously remounting
my component has this
`componentWillMount(){
changeMyReduxStateVariable();
}

....

mapStateToProps(state){
return {
variable:state.MyReduxStateVariable
}
}
return <Route key={index} exact={route.exact} path={route.path} component={userIsAdmin(route.component)}`

when i remove the userIsAdmin it works fine

[kjanoudi]

I have this issue as well @MDSecurity except I noticed it will consistently get componentWillReceiveProps, and never re-render.

[kjanoudi]

        if (this.props.isAuthenticated !== nextProps.isAuthenticated) {
          return true
        }
        if (this.props.isAuthenticating !== nextProps.isAuthenticating) {
          return true
        }
        return false
      }

did the trick for me too. thanks @mshick

[mjrussell]

Sorry all, was on vacation. I'll get a new version out by the end of the week

[mjrussell]

Ok I've got the code all done (I think) in #191. I definitely believe that an SCU is not the right approach generally because if you have the following:

      shouldComponentUpdate(nextProps) {
        if (this.props.isAuthenticated !== nextProps.isAuthenticated) {
          return true
        }
        if (this.props.isAuthenticating !== nextProps.isAuthenticating) {
          return true
        }
        return false
      }

This means that the component will not re-render any children if either of these props don't change. So if you connect "above" this wrapper (like connect(mapStateToProps)(authWrapper(MyComponent)) or render it inside another component that passes some props, the wrapped component will never see new prop changes. I think this would be complicated and tricky for users.

Instead, the default behavior for the router redirects is to use a new propMapper field for the authWrapper that by default drops the properties created specifically for the wrapper when passed to the wrapped component. This way, the wrapped component will never get passed properties which keep being not shallowly equal to each other and the redirection wrapper can be more easily viewed as a simply passthrough when the auth check passes.

Note that if you don't have a pure component or an SCU check with shallow equals, you can still wind up with unnecessary re-renders, depending on how the parent component (in many cases the router) passes down properties to the wrapped component, but that is true with or without redux-auth-wrapper.

I'd like to update the documentation before merging and cutting a new release, but if anyone wants to give it a try, that would be great. I've added a test case so I think I've got it covered, but would be nice to ensure it covers the cases you all found.

[tkvw]

I just installed the new version and it works for me, no more re-rendering. \o/

[tkvw]

I was cheering a bit to early, redirect is not passed down to the WrappedComponent, but there are still rerenders because connect passes down redirect to authWrapper which state changes because of this.

I think the solution provided @mshick works best.

Update: Actually now I read this is in your notes:

Note that if you don't have a pure component or an SCU check with shallow equals, you can still wind up with unnecessary re-renders, depending on how the parent component (in many cases the router) passes down properties to the wrapped component, but that is true with or without redux-auth-wrapper.

[mshick]

One idea: could you support passing a custom shouldComponentUpdate function to the auth wrapper, so we can set this up ourselves? I don't know if the propMapper abstraction is as necessary then.

[jakelazaroff]

I'm seeing the same issue as @MDSecurity — the components that I'm decorating aren't just re-rendering, but re-mounting as well. In that case, I don't think shouldComponentUpdate will work?

[senpaaii]

I too am seeing the same issue as @jakelazaroff and the current commit in PR doesn't resolve the situation as my component is re-mounting and thus I can't filter by shouldComponentUpdate.

My issue seems to be with using the redux-auth-wrapper multiple times through child components.
E.g. Switch Routes is wrapped by a userIsAuthenticatedRedir and a particular route is wrapped by another userIsAdminRedir. I've flattened the structure now so that a react tree branch only calls the auth methods once and it seems to have fixed my issue, although limits my intended use of the package.

[yeluolei]

same issue here, waste me a lot of time to find this problem..

[mjrussell]

Hey everyone,

Im very sorry for the long delay, I think a bit of an explanation is in order. I did a tremendous amount of work for my limited amount of free time getting version 2 pushed and through the door and was feeling accomplished but a little burnt out on completion. Then this re-render bug popped up and although #191 solved some cases, it really didn't solve the problems listed above. So I stepped away from redux-auth-wrapper and open source work to recharge a little bit.

Coming back, I had a small inspiration and think I've fixed the issue in #203. The test I wrote that I couldn't get fully working in #191 looks good in #203 so I think it should address any issues caused by redux-auth-wrapper. Thats not to say that react-router might throw in some props that cause re-rendering but adding in redux-auth-wrapper shouldn't trigger any additional re-renders. Please let me know if that is not the case, otherwise I'll merge the PR in soon and get a new release out there.

[jakelazaroff]

Just want to say thank you for putting in work on this — it's something you do in your free time, you shouldn't have to apologize for getting burnt out or wanting a break 🙂 we all appreciate it!

[oyeanuj]

@mjrussell Just wanted to second @jakelazaroff, we appreciate you putting out this super helpful library and being responsive! But your own health comes first.. so take care!

[yeluolei]

@mjrussell Just want to say thank you, we appreciate all your work, it's really help us a lot. Take care of yourself first, it's really the most important thing in your life. Sincerely. 😆

[mjrussell]

Thanks everyone for all the support, really appreciate it. Version 2.0.2 was just pushed with this fix so it should be all set!