componentWillReceiveProps optimisation

[nfcampos]

todo:

• fix current tests (all the failures are expected)
• test that mapPropsToRequestsToProps isn't called if neither props nor context actually changed
• make sure current tests cover mapPropsToRequestsToProps being called if either props or context actually changed (they already do)

[nfcampos]

if anyone can think of a better name for this._propsAndContextDidntChange I'm all ears :)

(I save it to avoid recomputing it shouldComponentUpdate (which, if props or context changed, always runs after componentWillReceiveProps))

[nfcampos]

I think I'm going back on this and I think this optimisation should apply only to componentWillReceiveProps — ie. whether to call refetchDataFromProps or not — and not do anything on shouldComponentUpdate — ie. whether to rerender the component or not.
Rationale is two fold:

• if we keep the optimisation confined to componentWillReceiveProps there's no need to make it optional, since it doesn't affect re-rendering the wrapped component
• while it's impossible for a user of react-refetch to apply the optimisation of componentWillReceiveProps without our explicit support, it's trivial for the user to apply the re-render optimisation, either use PureRenderMixin, write a shouldComponentUpdate on the wrapped component or wrap the wrapped component in eg. https://github.com/acdlite/recompose/blob/master/docs/API.md#pure before passing it to connect

What do you think @ryanbrainard ?

[ryanbrainard]

I think this optimisation should apply only to componentWillReceiveProps — ie. whether to call refetchDataFromProps or not — and not do anything on shouldComponentUpdate — ie. whether to rerender the component or not.

I'm not sure I 100% agree (mostly because I haven't thought about it enough), but I think that confining it to componentWillReceiveProps is a great start and nicely limits the scope of this PR. Mind rebase/merging with master and then we can pull this in?

[nfcampos]

I'm not sure myself, because for my uses I always want the component to be pure, but some use cases might require that it be non pure, so it'd probably have to be an option and I don't like options...

[nfcampos]

I've rebased

[ryanbrainard]

Released in v1.0.0-beta.2. Thanks @nfcampos !

Note, when I was testing this out, my app was still using React Router 1.0, so the router stuff was in props and my components where directly connected to the router, so that meant the optimization didn't actually do anything since props were always changing. Upgrading to 2.0 would probably help since things would move to context (of course unless context was also being used by connect), but it does show the need to potentially add a layer between the connected component and the outside world to only pass in the props that are actually needed. onlyupdateforkeys could also probably help here. Because if someone really needs this optimization, they would probably need to also use onlyupdateforkeys, I question its need a little bit (and hope it doesn't introduce subtle bugs), but I think it is probably the right thing to do, so pulled it in and let's see how it does in beta.

[nfcampos]

Good points.

This got me thinking about times when componentWillReceiveProps is called:
a) the connected component is rendered inside a component that has state — every time the state of the parent changes componentWillReceiveProps of the connected component will be called even if nothing about the props changed, see this gist I made real quick http://esnextb.in/?gist=4c7a4e4335d112eacab14c8f0ef5d6e6 (click execute, probably only works in chrome)
b) same as above but the connected component is rendered with children — gist: http://esnextb.in/?gist=8d2073bb6513d796670665685b38df4f
c) the component wrapped by connect defines contextTypes but the connect function doesn't use context (it's the wrapped component itself that needs the context) (edit: forget the context case, see issue #99, the way I'd fix that would make sure the component never gets contextTypes if mapPropsToRequestsToProps doesn't depend on context)

The optimisation in this PR takes care of (a) and (c). (b) would actually be good to take care as well, I don't think there's any sane use case in which mapPropsToRequestsToProps uses props.children so we could just omit children in the shallowEqual comparison. What do you think about this?

As for subtle bugs, basically they'll come down to times when people write mapPropsToRequestsToProps functions that are either not pure (eg. depend on document.location) or when they change something their props in a way that maintains shallow equality, let's be on the lookout for that, but what would you want to do if such issues appeared, remove the optimisation or make it optional?

Directly doing something like

onlyUpdateForKeys(['this', 'that'])(connect( props => ... )(SomeComponent))

sounds perfectly reasonable but I think the cases above make this more primitive optimisation worth it, what do you think?

[ryanbrainard]

Interesting point about children. One thing we could do is to base equality on the children's keys, as suggested in https://stackoverflow.com/questions/28784050/react-how-to-compare-current-props-children-with-new-one. I did a bit of googling, but was kind of surprised this isn't a problem more people have run into. I would have expected that React would have an out of the box util for comparing nodes. Do you know of something?

cases above make this more primitive optimisation worth it

If we did something like this, are you thinking something that would go into defaults?

Side note: This does make me question again a bit of we should be stuffing connect-level only things inside of defaults. I know we just did work to merge options into defaults, and I think that was the right move for all the things that can actually be in connect or mappings, but for things that are truly only for connect, I wonder if it could be confusing (both for apps and internal use). Thoughts on this?

[nfcampos]

• re: children, comparing children is basically what React does internally every time render is called to decide whether anything in the DOM needs to change, so I don't think we should try to do that ourselves (especially because children is kind of an array of objects that can contain other children props, so comparing children is basically a deepEqual comparison of two objects, which is pretty expensive. Comparing just keys isn't enough because you might have a set of children with the same keys but different props. But for componentWillReceiveProps I don't think there's any value to comparing children, you can't use them inside mapPropsToRequestsToProps so you don't care whether it changed I think. (If we're talking about something for shouldComponentUpdate then the fact that comparing children is too expensive still applies, I think when children changes you have to call render again and that's it)
• as for mixing defaults and options I agree that it is confusing to define them in the same object/method. But I think the idea of defaults as a method that you call and which returns a new connect with the defaults applied is superior to the previous way of setting settings as the 2nd arg to connect because settings might also be something you want to share across your whole app. So maybe another one of these methods called options for setting these options like withRef?

[ryanbrainard]

Agree on all points.

You're right, children should be opaque to connect -- let's just omit it by default. If there's a legitimate case, maybe we can have some option to consider it.

I also agree on a chainable options for connect-level things things like withRef and maybe an onlyUpdateForKeys-like option. I think splitting options and default will be clearer for everyone.

[nfcampos]

I've opened a PR with the .options() method. How do you want to go about omitting children, do we use eg. omit from lodash or do we add that functionality to our shallowEqual?

[nfcampos]

@ryanbrainard any opinion on the above, whether to use a dependency for that or not?

[ryanbrainard]

@nfcampos i was hesitant to pull in more deps, but that is probably better to use something like lodash for this and also for shallowEquals rather than maintaining our own. react-redux is also depending on it now, so that definitely makes me feel more comfortable doing it.

[nfcampos]

@ryanbrainard sadly lodash doesn't have a
shallowEqual so that one we should probably keep ours, but I'll use lodash for omit to omit children. Another thing, I've now seen the pattern connect(...)(pure(Component)) enough times in my code that I think it's worth it to add in the shouldComponentUpdate optimisation to react-refetch (since mapPropsToRequestsToProps is already supposed to be a pure function, it's very likely that a significant part of the users also make their wrapped components themselves pure functions...). Do you agree with adding it? Should it live behind an option pure like in react-redux?

[ryanbrainard]

@nfcampos Sure, have at it :)