Make "key" optional for performance

[ryanflorence]

Right now we assign a key to a handler so that transitions from /users/1 to users/2 rerenders as expected without any extra work for the developer. However, we cause react to build up brand new dom with that transition.

I would like to do something like <Route useKey={false}/> or Router.disableKeys().

This would require devs to implement componentWillReceiveProps on their route handlers when transitioning from /users/1 to /users/2, but they will also get some perf improvements if they feel like they need them.

Thoughts?

[sophiebits]

If we make data fetching part of the router by default, I'd push for not assigning keys by default since components will be purer by default and the router can take care of re-fetching and passing the new data down (which is the logic that you normally might have in a componentDidUpdate).

[brigand]

This one has been biting me a bit, because I have a scrolling list on the left, and the details for the selected item on the right.

I need the url to update when you click a new item, which works, but then the list on the left is scrolled back to the top because the dom was thrown out. In reality, all that's changed is the text content of ~10 elements on the right hand side.

[ryanflorence]

yeah thats 👎 and exactly the case where this option would be 👍

[mjackson]

Why don't we just make this a static method of the route handler? Default behavior would be to use no key, but the user could provide a getRouteKey(params, query) method that they could use to generate a key for that route if they want.

[sophiebits]

Since users call activeRoute manually now, they can already pick a key themselves if they want.

[brigand]

Wow, thanks @spicyj. That fixed my UX problem.

In that case, it should just be a note in the README if anything.

[ryanflorence]

@spicyj and I have talked in irc about this a bit. When #57 is all sorted out, the implementation there may make it so we can just remove keys altogether.

The reason we have them is to transition from and to the same route with different params; if we prescribe a convention around providing data, we will necessarily implement the hooks that will cause the view to update (w/o needing keys).

[ryanflorence]

<Routes useAutoKeys={false}/>

?

[sophiebits]

Flags that change how the entire application behaves are hard to reason about, especially if the default allows you to be sloppier when writing the original code – when you switch the flag later on you'll have no idea what will break. I don't have a great idea for a better plan here though. Perhaps now that AsyncState exists we can encourage using that and never add keys?

[ryanflorence]

alright, will reopen if we head down that path and decide to come back to discuss this some more.

[ryanflorence]

reopening, I'm starting to think we should not be doing anything with keys.

[mjackson]

Why not? I honestly have no idea.

[ryanflorence]

We added the key in because a transition from /users/1 to /users/2 doesn't do anything unless you implement componentWillReceiveProps. At the time, we were react newbies and thought "that sucks!" so we added keys and were happy that everything worked.

My only concern is getting lots of tickets around "transitioning to the same route is broken". I guess we can just make it loud and clear in the README, examples, docs, everywhere, that you need componentWillReceiveProps when you are using dynamic segments.

[mjackson]

Ah, now I remember.

Can we do something fancy with shouldComponentUpdate? Maybe something like:

shouldComponentUpdate: function (nextProps, nextState) {
  return compareParams(this.props.params, nextProps.params);
}

?

[sophiebits]

Yay! I don't think shouldComponentUpdate helps here though.

[gaearon]

I think it's fair to have people implement componentWillReceiveProps because it perfectly mirrors what happens when you pass a different value were you to implement routing yourself. Moreover if you don't want behavior, it's trivial to give your route handler a key yourself and opt-in to full replacement, just like in normal React code. I'd like to see keyless as default.

[machty]

Not sure if this is possible in React but one thing that might be helpful would be to warn if, for a route that introduces a new param relative to its ancestors, componentWillReceiveProps hasn't been defined.

[ryanflorence]

follow #229