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

Infinite loop due to push? #44

Closed
rpribadi opened this issue Jun 11, 2016 · 14 comments
Closed

Infinite loop due to push? #44

rpribadi opened this issue Jun 11, 2016 · 14 comments
Labels

Comments

@rpribadi
Copy link

Hi @mjrussell , I'm using your package in my project and I stumble on this issue where it creates an infinite loop between the new and old url.

For example:
I have a component, ProductDetailComponent with the acceptable Route url
'/product-detail/:id'

I wrapped the component with UserIsAuthenticated

import { UserAuthWrapper } from 'redux-auth-wrapper';
import { routerActions } from 'react-router-redux';


export const UserIsAuthenticated = UserAuthWrapper({
  authSelector: (state) => { return state.authentication; },
  redirectAction: routerActions.replace,
  wrapperDisplayName: 'UserIsAuthenticated',
  failureRedirectPath: '/authentication',
  predicate: (user) => { return user.accessKey; }
});

At the first load, I go to /product-detail/1 url, and since the User is not authenticated yet, so I got redirected to /authentication to do the login, and upon successful login, I got redirected again to the original page, /product-detail/1. So far so good.

Now let's say, in the ProductDetailComponent, there's a selectbox to select similar Product. If the user select another similar product, then the app will trigger an action that will 'restart' the component:

export function restartModule(productId) {
  return (dispatch) => {
    dispatch(push('/product-detail' + productId));
    dispatch(resetSettings());
    dispatch(fetchProductDetail(productId));
  };
}

And that's when the infinite loop happened.

I tried to debug it but I didn't get far enough. However, if I commented
dispatch(push('/product-detail' + productId));
the infinite loop didn't happen and application works fine (except the url is still the same, doesn't change to the new productId url).

From my understanding, from the Route configuration, regardless which url that I access:

/product-detail/1
/product-detail/2
/product-detail/3

the app will always mount ProductDetailComponent because they match its url pattern /product-detail/:id, and UserIsAuthenticated should be checked only once, since we are not changing component. So why does the UserIsAuthenticated for ProductDetailComponent get triggered again if push to /product-detail/2 or so on?

Other information, I use react-redux-starter-kit with:

    "react": "^15.0.0",
    "react-dom": "^15.0.0",
    "react-redux": "^4.0.0",
    "react-router": "^2.2.0",
    "react-router-redux": "^4.0.0",
    "redux": "^3.0.0",
    "redux-auth-wrapper": "^0.5.0",

Any pointers to fix this issue? Thanks in advance!

@mjrussell
Copy link
Owner

@rpribadi

Thats strange, and I know how hard infinite redirection loops can be to debug. Can you share your route configuration file with the auth wrappers being applied? Or are you applying them with class decorator style?

If possible a link to your code or a minimal example demonstrating this behavior would be really helpful.

@mjrussell mjrussell added the bug label Jun 11, 2016
@mjrussell
Copy link
Owner

I may have actually created an example that also displays strange infinite looping. Interestingly, it only happens on React 15.1 and not 15.0.2. I'm going to investigate further but could you try pinning your react dependencies (react, react-dom, and other addons) to 15.0.2 and see if you still have this issue?

@mjrussell
Copy link
Owner

The issue I found looks to be related to a change in behavior of CSSTransitionGroups in React that I had in my example. Unless you are using that, its unlikely to be the auth wrapper (but possible). If you can show a minimal example that is still using this issue that would be fantastic!

@rpribadi
Copy link
Author

@mjrussell I don't use CSSTransitionGroups in project, so it's unlikely because of that. In the meantime, let me follow your suggestion first, install the version 15.0.2, and see how it goes.

Also, let me prepare a simplified version of the project. I'll get back to you once it's done.

Thanks!

@rpribadi
Copy link
Author

@mjrussell , I updated the dependencies to:

    "react": "15.0.2",
    "react-dom": "15.0.2",
    "react-router": "2.4.1",
    "react-router-redux": "4.0.0",
    "react-redux": "^4.0.0",
    "redux": "^3.0.0",

but no luck. The infinite loop still happening.

I'll continue with preparing the simplified version.

@mjrussell
Copy link
Owner

@rpribadi ok thanks for the update!

@rpribadi
Copy link
Author

rpribadi commented Jun 13, 2016

@mjrussell , I've prepared the simplified version, and I notice different behaviour between authenticated component and not, that further down the hill might trigger this issue.

I originally built my project without having my component wrapped with redux-auth-wrapper, together with some logic implemented in the component life cycle functions like componentWillMount and componentWillUnmount, and I assume, if I wrapped my component with redux-auth-wrapper, the life cycle behaviour remains.

For example, let's say these are the flow that are triggered by user:

  1. /product/1
  2. /product/2

Without being wrapped, my app will behave like this:

  1. /product/1 is accessed
  2. ProductComponent.componentWillMount
  3. ProductComponent.render
  4. /product/2 is accessed
  5. ProductComponent.render

With being wrapped, my app will behave like this:

  1. login first!
  2. /product/1 is accessed
  3. ProductComponent.componentWillMount
  4. ProductComponent.render
  5. /product/2 is accessed
  6. ProductComponent.componentWillUnmount
  7. ProductComponent.componentWillMount
  8. ProductComponent.render

As you can see, the main difference here is redux-auth-wrapper will unmount the current ProductComponent first, then mount a new ProductComponent, hence step 6&7.

This behaviour is critical in my project implementation. For example, initially I just need to download reusable data (eg: list of all the products) once, and then, changing the selected product will only cost me fetching the product detail, but if I implement redux-auth-wrapper, it will unmount and mount the component, and cost me reloading all the product list again (because I hook it on the componentWillMount).

I think I'm getting somewhere. I'll debug my original project for now. In the meantime, you can test the infinite loop behaviour here.

Thanks!

@mjrussell
Copy link
Owner

@rpribadi Thanks for the example repo, Im going to work this tonight and tomorrow. I also got some clarification from the React devs on my issue and I think it could be possibly related to what you are running into (facebook/react#7025). Im going to tweak the auth-wrapper based on their feedback and see if your repo still has the problems you described.

@mjrussell
Copy link
Owner

mjrussell commented Jun 16, 2016

@rpribadi I think the reason you may be running into this issue with the unmounting/remounting is because you are applying the auth-wrapper at a level too low in the route tree. I don't believe there is anything I can do from an HOC standpoint to prevent the unmounting/remounting when you apply the auth wrapper at the ProductComponent.

One thing I would try is to instead apply the auth wrapper a level above. So you would end up with a routing structure more like (simplified your routing structure for easier discussion):

<Route path='/'>
  <Route path='authentication' component={Authentication} />
  <Route path='product' component={UserIsAuthenticated(ProductWrapper)}>
    <Route path=':productId' component={Container} />
  </Route>
</Route>

You might also try experimenting with moving the application of the HOC outside of getComponent. An HOC will return a new component each time it is applied, and react might treat these as unique components and therefore has to unmount one before mounting the other.

Instead of doing cb(null, UserIsAuthenticated(Container)); in the getComponentcall, try defining outside the "route creator":

const AuthenticatedContainer = UserIsAuthenticated(Container)

and inside getComponent:

cb(null, AuthenticatedContainer);

(Edit sorry accidentally submitted early)

@rpribadi
Copy link
Author

@mjrussell You are right! Moving out the wrapping of the container outside getComponent function solves componentWillMount and componentWillUnmout issue. But I'm not really sure about the impact down the road.

import { injectReducer } from '../../store/reducers';
import UserIsAuthenticated from 'commons/Auth';
import Container from './Layout';

const AuthenticatedContainer = UserIsAuthenticated(Container);

export default (store) => ({
  path: 'product/:id',
  /*  Async getComponent is only invoked when route matches   */
  getComponent(nextState, cb) {
    /*  Webpack - use 'require.ensure' to create a split point
     and embed an async module loader (jsonp) when bundling   */
    require.ensure([], (require) => {
      /*  Webpack - use require callback to define
       dependencies for bundling   */
      const reducer = require('./reducers').default;

      /*  Add the reducer to the store on key 'counter'  */
      injectReducer(store, { key: 'product', reducer });

      /*  Return getComponent   */
      cb(null, AuthenticatedContainer);

      /* Webpack named bundle   */
    }, 'product');
  }
});

Initially, I loaded the Container just like how I loaded the reducer using require callback passed by require.ensure

    require.ensure([], (require) => {
      /*  Webpack - use require callback to define
       dependencies for bundling   */
      const reducer = require('./reducers').default;

     // end the rest of the function here...

    }, 'product');

And I'm not really sure what's the impact in there. Will the async getComponent become partially asynchronous?

As always...Thanks for your help.

@mjrussell
Copy link
Owner

@rpribadi Glad we are finding the root cause of this!

Yeah moving it outside and without the require.ensure means that it won't be lazily evaluated and wouldn't get split into bundles if you have that setup in your webpack config. I think the only thing you could do to preserve the lazy eval would be to move the application of the HOC into the container file you have.

So in the case of your example, applying the HOC in the Layout/containers.js file. So you might have something like this:

import React from 'react';
import { connect } from 'react-redux';

import UserIsAuthenticated from '../commons/Auth';
import Component from './components';
import {fetchInitialSettings, resetProductDetail, setActiveProduct, restartModule, resetSettings} from '../actions';
import {getProductDetail, getSimilarProductList} from '../selectors';

const mapStateToProps = (state, ownProps) => {
  return {
    similarProductList: getSimilarProductList(state),
    product: getProductDetail(state),
    productId: parseInt(ownProps.params.productId)
  }
};

const mapActionCreators = {
  fetchInitialSettings: fetchInitialSettings,
  restartModule: restartModule,
  resetProductDetail: resetProductDetail,
  setActiveProduct: setActiveProduct,
  resetSettings: resetSettings
};

export default UserIsAuthenticated(connect(mapStateToProps, mapActionCreators)(Component));

And your Product/index.js goes back to: cb(null, Container);

This way the wrapper is still applied only once to your component each time getComponent is invoked.

@mjrussell
Copy link
Owner

@rpribadi did the latest suggestion work for you? Can this be closed now?

@rpribadi-ds
Copy link

@mjrussell , I followed your suggestion to move it into containers.js and it works. Awesome! Thanks for your help.

@yeluolei
Copy link

yeluolei commented Sep 8, 2017

have the same problem when use my own async loader,thanks for the suggestion.

the final suggestion is, if you are use async component, wrap it with the component itself, not after load.

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

4 participants