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

Redux auth wrapper/redux-thunk combo doesn't respect hierarchy #101

Closed
Blackclaws opened this issue Dec 14, 2016 · 7 comments
Closed

Redux auth wrapper/redux-thunk combo doesn't respect hierarchy #101

Blackclaws opened this issue Dec 14, 2016 · 7 comments
Labels

Comments

@Blackclaws
Copy link

So I've already started an issue over at the redux thunk repo:
reduxjs/redux-thunk#119

The basic idea is that I have a component that needs to access state with mapStateToProps. The state it needs to access is only provided if an auth-wrapper returns true, i.e. that is my guard against rendering the component with state that it could consider "broken". Now if I do normal redux state updates everything works fine. The wrapper detects the change and redirects without rendering the component.

Now if I do the same state update from within a thunk i.e. asynchronously then it breaks down. The state which my component needs access to gets unset by the action (as it should be) and then an error occurs.

In this case the auth wrapper doesn't protect the component form rerendering neither does it redirect afterwards because the whole state update breaks:

Uncaught (in promise) TypeError: Cannot read property 'username' of null
at AlreadyLoggedInComponentBare (AlreadyLoggedInComponent.js:29)
at StatelessComponent.render (ReactCompositeComponent.js:44)
at ReactCompositeComponent.js:796
at measureLifeCyclePerf (ReactCompositeComponent.js:75)
at ReactCompositeComponentWrapper._renderValidatedComponentWithoutOwnerOrContext (ReactCompositeComponent.js:795)
at ReactCompositeComponentWrapper._renderValidatedComponent (ReactCompositeComponent.js:822)
at ReactCompositeComponentWrapper._updateRenderedComponent (ReactCompositeComponent.js:746)
at ReactCompositeComponentWrapper._performComponentUpdate (ReactCompositeComponent.js:724)
at ReactCompositeComponentWrapper.updateComponent (ReactCompositeComponent.js:645)
at ReactCompositeComponentWrapper.receiveComponent (ReactCompositeComponent.js:547)
AlreadyLoggedInComponentBare @ AlreadyLoggedInComponent.js:29
StatelessComponent.render @ ReactCompositeComponent.js:44
(anonymous) @ ReactCompositeComponent.js:796
measureLifeCyclePerf @ ReactCompositeComponent.js:75
_renderValidatedComponentWithoutOwnerOrContext @ ReactCompositeComponent.js:795
_renderValidatedComponent @ ReactCompositeComponent.js:822
_updateRenderedComponent @ ReactCompositeComponent.js:746
_performComponentUpdate @ ReactCompositeComponent.js:724
updateComponent @ ReactCompositeComponent.js:645
receiveComponent @ ReactCompositeComponent.js:547
receiveComponent @ ReactReconciler.js:125
_updateRenderedComponent @ ReactCompositeComponent.js:754
_performComponentUpdate @ ReactCompositeComponent.js:724
updateComponent @ ReactCompositeComponent.js:645
receiveComponent @ ReactCompositeComponent.js:547
receiveComponent @ ReactReconciler.js:125
_updateRenderedComponent @ ReactCompositeComponent.js:754
_performComponentUpdate @ ReactCompositeComponent.js:724
updateComponent @ ReactCompositeComponent.js:645
performUpdateIfNecessary @ ReactCompositeComponent.js:561
performUpdateIfNecessary @ ReactReconciler.js:157
runBatchedUpdates @ ReactUpdates.js:150
perform @ Transaction.js:140
perform @ Transaction.js:140
perform @ ReactUpdates.js:89
flushBatchedUpdates @ ReactUpdates.js:172
closeAll @ Transaction.js:206
perform @ Transaction.js:153
batchedUpdates @ ReactDefaultBatchingStrategy.js:62
enqueueUpdate @ ReactUpdates.js:200
enqueueUpdate @ ReactUpdateQueue.js:24
enqueueSetState @ ReactUpdateQueue.js:209
ReactComponent.setState @ ReactComponent.js:63
handleChange @ connect.js:302
dispatch @ createStore.js:186
dispatch @ VM119:1
(anonymous) @ redux-logger:89
(anonymous) @ middleware.js:22
(anonymous) @ redux-thunk:14
dispatch @ applyMiddleware.js:45
(anonymous) @ reducer.js:81

@mjrussell
Copy link
Owner

@Blackclaws can you include some code, specifically:

  • the creation of the wrapper
  • AlreadyLoggedInComponent/AlreadyLoggedInComponentBare
  • The code where you apply the auth wrapper to the component?

Best case would be a minimal example but code would be a great start. Without seeing anything, Im wondering if you are connecting the userData in multiple places: one for the redux-auth-wrapper, and one for the already logged in and the values are inconsistent

@Blackclaws
Copy link
Author

@mjrussell Sure:

Wrapper:

const isAuthenticated = userAuthWrapper({
  failureRedirectPath: '/debug/login',
  authSelector: state => state.auth,
  redirectAction: routerActions.replace,
  wrapperDisplayName: 'IsAuthenticated',
  predicate: auth => auth.isAuthenticated === true,
});

AlreadyLoggedInComponent

export function AlreadyLoggedInComponentBare({ dispatch, user, authClient, intl }) {
  const handleLogout = () => {
    //dispatch(actionSucceedLogout());
    dispatch(logout(authClient));
  };

  return (
    <div>
      <div className="row column">
        <AlertBox
          message={intl.formatMessage(
            {
              id: 'auth.message.loggedInAs',
              defaultMessage: 'You are logged in as {username}.',
            },
            { username: user.username },
          )}
          messageType="success"
          hide={false}
        />
        <button className="float-right button" onClick={handleLogout}>
          <FormattedMessage
            id="auth.logout"
            defaultMessage="Logout"
          />
        </button>
      </div>
    </div>
  );
}
AlreadyLoggedInComponentBare.propTypes = {
  dispatch: React.PropTypes.func.isRequired,
  authClient: React.PropTypes.instanceOf(AuthClient),
  user: React.PropTypes.shape({
    username: React.PropTypes.string.isRequired,
  }).isRequired,
  intl: intlShape.isRequired,
};

export default connect(state => ({
  user: state.auth.user,
  loginFailed: state.components.auth.login.loginFailed,
}))(injectIntl(AlreadyLoggedInComponentBare));

Where dispatch(logout(authClient)) is an async action that sets isAuthenticated to false and sets user to null.

The default export here is AlreadyLoggedInComponent.

The auth wrapper is applied on top of another wrapper:

function AlreadyLoggedIn() {
  const authClient = new AuthClient(settings.server.apiUrl);
  return (
    <AlreadyLoggedInComponent authClient={authClient} />
  );
}

since we are injecting the authClient ( this could also be done on the authWrapper as it passes through, however not every instance of AlreadyLoggedInComponent might need an authWrapper).

The authWrapper is then applied using:

<Route path="/debug/already-logged-in" component={isAuthenticated(AlreadyLoggedIn)} />

@mjrussell
Copy link
Owner

@Blackclaws each wrapped component with redux-auth-wrapper receives the result of the authData selector. Could you try instead of connecting the user from the state in the already logged in component, take it from the props? This would mean you need to use the AlreadyLoggedIn component with an auth-wrapper or with a connect when it is not immediately wrapped by an auth wrapper.

Something like:

export default connect((state, ownProps) => ({
  user: ownProps.authData.user,
  loginFailed: state.components.auth.login.loginFailed,
}))(injectIntl(AlreadyLoggedInComponentBare));

@Blackclaws
Copy link
Author

Getting the information from authData does work, and was how the component was set up before. However that would also mean that this component cannot exists separately from the AuthWrapper which can be desired if a whole branch of router is protected by an AuthWrapper. I would much rather have expected that a rerender is forced and the component isn't even rendered anymore when the state changes to not fulfill the predicate anymore.

@mjrussell
Copy link
Owner

I agree this is not an ideal behavior, but I think that this is ultimately an issue with what guarantees react-redux can provide with connected components.

@mjrussell
Copy link
Owner

mjrussell commented Mar 29, 2017

@Blackclaws I know this is quite old, but it appears that this may be fixed in the latest react-redux (5.x): reduxjs/react-redux#416 (comment)

@mjrussell
Copy link
Owner

Closing since this has been fixed in the latest react-redux release

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