router5 / react-router5

Helpers for using router5 with React [MOVED]
MIT License
83 stars 8 forks source link

Component not re-rendered when changing url param #27

Closed LeonardoGentile closed 7 years ago

LeonardoGentile commented 7 years ago

Salut, thank you very much for this 👍

I'm starting to set up an application and I've noticed a problem, not sure if this a mistake I made or a bug.

These are my routes

export default [
  { name: 'home', path: '/' },
  { name: 'login', path: '/login'},
  { name: 'index', path: '/index/:id'},
];

I've followed the example and my Main component is something like:

const components = {
    'home':   Home,
    'index':  Index,
    'login':  Login,
};

class Main extends React.Component {
  render(){
    const { route } = this.props;
    const segment = route.name.split('.')[0];
    return createElement(components[segment]);
  }
}
export default routeNode('')(Main);

Then all my components are exported wrapping them with routeNode HOC.

I'm using the loggerPlugin.

So if I navigate from any route to /index/1 everything is ok and I can see the correct log in the console. If from there I go to /index/2 then the logger says correctly

Transition started from state
Object {name: "index", params: Object, path: "/index/1", meta: Object}
To state
Object {name: "index", params: Object, path: "/index/2", meta: Object}
Transition success

but the render method of my Index component is not called. From here changing any id pram in my index route will not call the render anymore.

I've set the router option to be strictQueryParams: true but nothing change. Am I missing something?

LeonardoGentile commented 7 years ago

I am new to React but I would say the problem comes from here. Inside the routeNode HOC render method:

render() {
    const { props, router } = this;
    const { previousRoute, route } = this.state;
    const component = createElement(
        RouteSegment,
        {...props, router, previousRoute, route }
    );

    return component;
}

This always tries to create a New component. But in this case it is the same component that existed previously so probably the internal react algo for virtualdom comparison says that this component already exists and there is no need to re-create it. Indeed we should not re-create it but call again its render method because only its props are supposed to be changed. Or maybe this is the problem, meaning that in my case I'm not passing any props, so actually nothing changed apart the router injected props via context.

@troch any tough on this or am I saying stupid things? :laughing:

troch commented 7 years ago

@LeonardoGentile sorry I had missed that notification somehow. There are a few things you can do:

I recommend passing route parameters as props and adding a key to components (especially if you ever use reload option of router.navigate).

class Main extends React.Component {
  render(){
    const { route } = this.props;
    const segment = route.name.split('.')[0];

    return createElement(
        components[segment],
        {
            key: route.meta.id,
            ...route.params
        }
    );
  }
}
export default routeNode('')(Main);
LeonardoGentile commented 7 years ago

@troch thank you for your time 👍 At this point it's not clear to me how the react createElement works, if I've understood:

I couldn't find clear docs about this. I guess this is because most of the times react example just use jsx that internally will call createElement, isn't it?

Anyway, back to your solution, it kind of work. 1) If I pass both {key: route.meta.id, ...route.params} then there will be a full unmount/mount and things work as expected 2) If I just pass {...route.params} then the render method is indeed called again BUT props.route is not up-to-date. It shows the old route with the old param.

I would like to use the 2nd solution to avoid to have a full re-render all the time but there is something still not working.

Anyway, considering this is a common case, I guess this should be added to the doc somewhere?

troch commented 7 years ago

Did you use the examples as a starting base?

LeonardoGentile commented 7 years ago

yes, of course now it's a bit more complex than that.
Digging down I can see that the nodeListener handler is called for the Main component but NOT for its children (I'm still talking about the /index/:id example when id changes from 1 -> 2) so the function setState is never called for them when I only pass {...route.params} (for triggering a re-render but not a full unmont/mount)

 componentDidMount() {
    ifNot(
        this.router.hasPlugin('LISTENERS_PLUGIN'),
        '[react-router5][routeNode] missing listeners plugin'
    );

    // This is called only for the Main component, NOT for its children components
    this.nodeListener = (toState, fromState) => this.setState({ previousRoute: fromState, route: toState });

    this.router.addNodeListener(nodeName, this.nodeListener);
}
troch commented 7 years ago

If you are using examples, the problem comes from a dev dependency which was never updated: router5.transition-path. It should be ^4.0.0 and not ^3.0.0. You probably don't need to have this dependency (it's used in one of the cycle.js example), but by having it it caused router5 to not properly resolve one of its internal dependency.

LeonardoGentile commented 7 years ago

I see, but I copy pasted the relevant parts on my separate project so I am not importing directly router5.transition-path and using these:

"react-router5": "^4.0.1",
"router5": "^4.5.1"
troch commented 7 years ago

The React example had a bug (I just fixed it), similar to if not exaclty what you described. Can you log the following?

npm ls router5.transition-path
LeonardoGentile commented 7 years ago
└─┬ router5@4.5.1
  └── router5.transition-path@4.0.1
troch commented 7 years ago

Digging down I can see that the nodeListener handler is called for the Main component but NOT for its children (I'm still talking about the /index/:id example when id changes from 1 -> 2)

index is a top route, so going from index(id: 1) to index(id: 2) will only reload routeNote('')(Component). Do you have your code available somewhere? Seeing the whole lot will help me see the issue.

LeonardoGentile commented 7 years ago

Hello, I've cleaned up a bit my really horrible dirty example, you can find it here: https://github.com/LeonardoGentile/react-mobx-router5-example Just npm install then npm start.

If you manually (from the link box) navigate to /index/1 then to /index/2 you will see the problem I have described above. Indeed when the index page is accessed with a different id at least once it an object will be added to the store and printed to the page (using mobx stores and actions). So accessing it with id=1 then id=2 should print 2 objects. This does not happens.

The problem goes away if you navigate from anywhere to /index/1 the somewhere else, like home or login then to /index/2

Let me know if you have any trouble testing it or if something is not clear

troch commented 7 years ago

Thanks for this, I've cloned it and ran it. I need a bit more time to investigate it, will come back to you as soon as I can.

troch commented 7 years ago

So I think I found the issue in your code. It is in Index.jsx: you use routeNode('index')(Index); which injects route prop to your component but never re-renders since index doesn't have any child routes (and therefore cannot be a re-rendering node): as a result, the route used is the route when Index is mounted for the first time. That also explains why using key works..

LeonardoGentile commented 7 years ago

Oh I see, it's because it is associated with a node that doesn't have any sub-branches so it will never be a transition node. Well you are right, but I would say this is a common use case (to navigate to the same path but with different params) so I would consider the node itself a transition node. Isn't it?

IMHO it should exists a config option that when set would tell router5 to consider a node as a transition node when we transition to it, from it.

Wouldn't it be logic?

troch commented 7 years ago

I would say this is a common use case (to navigate to the same path but with different params)

It is super common indeed :). But it is the parent node which needs to be re-rendered (the node outputting your <Index /> component): in your case you only need routeNode(''). That's how the router works, how it treats route segments and nodes. For instance consider the following: your index route is protected with a canActivate method, based on its param id. When you go from id=1 to id=2, you still want canActivate to be called: you deactivate the current node (id=1) and activate a new one (id=2), the parent node being the transition node.

LeonardoGentile commented 7 years ago

Oh, I see, it took me a while to understand it. I've removed routeNode from index and it worked.

Maybe an highlight on the doc would save another headache 😄

Thanks 👍