RFC: Should update child context

[sebmarkbage]

See #2517

[eplawless]

@sebmarkbage @chrisui

Re: shouldUpdateChildContext, I think it's a useful addition. By default shouldComponentUpdate returns true, which makes it the responsibility of the child to determine whether something has changed. Implementing shouldComponentUpdate means that you're taking away that responsibility in the case where the parent knows better. The same is true of shouldUpdateChildContext, it only exists to take away responsibility from its children as an optimization.

Re: not broadcasting to the whole subtree, I think there are two valid patterns. Wormholes, as you suggested in #2517, make perfect sense. We use that pattern for grouping items within various subsystems (like focus and voice). Additionally, though, we'd want to use context to pass down i18n information to a whole section of (perhaps the entirety of) our application, and force a re-render of anything that uses it. That kind of broadcast pattern is probably relatively rare compared to the wormhole pattern, but I believe it's still valid.

Right now we're occasionally using mixins for focus and voice groups from the same source. I think there are valid use cases for not forcing a 1:1 mapping, though I understand the desire for the restriction.

[sebmarkbage]

Well currently the broadcast pattern can be implemented as wormholes as well. It's just more boilerplate, which might be ok if it is rare.

shouldComponentUpdate currently takes all your inputs (props, state, context). It is used to determine if any of your inputs changed in a unified way. This includes state. Then getChildContext get called IF one of the inputs changed. This means that a parent component already has the ability to stop child context from updating.

Any parent component that doesn't provide this context cannot intercept it with neither shouldComponentUpdate nor shouldUpdateChildContext so they're irrelevant.

The only benefit shouldUpdateChildContext provides is a way to update your props without updating your context. However, shouldn't you then be able to update your context without updating your props?

We don't allow you to make fine grained shouldComponentUpdate on a per prop level. It seems like this kind of fine grained optimization is better suited for generic subscriptions like Stores, Observables, etc. We generally avoid that strategy and optimize towards very coarse grained update strategies rather than fine grained subscriptions by default.

[eplawless]

Re: shouldUpdateChildContext I buy your argument, if it's a redundant mechanism or opens up those conceptual problems then maybe it's better to avoid it.

For sure, you could do broadcast with wormholes. Taking it to the extreme you could do it with props, too :) Potentially that boilerplate is fine, but I'd always lean toward avoiding boilerplate if it's reasonable to do so.

[eplawless]

Just to explore the boilerplate:

<I18NProvider>
  <List>
    <Item />
  </List>
</I18NProvider>

Let's say Item is using context.i18n, and you want to add a localized heading to List. If you opt in to context.i18n in List you've got to declare childContextTypes and getChildContext so it's passed along properly. If you neglected to forward context, presumably Item would then warn that its context wasn't provided, unless it was marked as optional in which case it would silently not work.

[jimfb]

@sebmarkbage Since children is a prop, this would mean that the ContextProvider (which is presumably a parent (but not an owner) of a very large subtree) would return true any time anything on the page changed. If context is assumed to have changed on every rerender, this means the ContextProvider effectively has no way of opting out of the context change, since it needs to update because the children have changed (ie. we don't want to notify all the context recipients deep in the tree, but we must). I think the common case is that the context does not change but the children prop does change and thus a render needs to happen but we don't want all the recipients of the context variable to start rerendering themselves.

[jimfb]

Context isn't supported yet; I don't think we want to have an example. For now, we will want to be unit-tests only.

[chrisui]

Yeah, this was just the branch I was working on for testing my stuff visually and so all this code would need cleaning up a lot before actually going in. Seb opened up a pr for discussions sake :)

It's great to see all the discussion which has been going on though! I've been busy and away over the past week so haven't been able to get involved too much. Hopefully will have a read through everything later!

[gaearon]

I’m closing in favor of discussion in #2517. We are cleaning up stale PRs. Also, there are plans to significantly change the core (#6170) so this will probably get less relevant in a few months. Definitely the issue is still there, but how we approach or implement the context internally may change.