Allow disabling Relay container shouldComponentUpdate

[peterhorne]

This PR introduces an optional flag pure to RelayContainer which overrides shouldComponentUpdate to return true.

Closes #674.

[josephsavona]

edit: nevermind, this makes sense after reading the redux docs at https://github.com/reactjs/react-redux/blob/master/docs/api.md#arguments

[peterhorne]

This is written as a double negative to handle the case when spec.pure is undefined and it should default to true.

[josephsavona]

yup, realized that after - makes sense

[wincent]

Instead of making this configurable, could we instead make our this.context vs nextContext check a little more sophisticated?

At the moment, we short-circuit with true as soon as we see Relay-specific context has changed. But if you look at what we do for non-query props (ie. non-Relay props) we also return true if any of them don't match. I wonder if we could/should do the same with context.

[peterhorne]

That would be ideal but I don't think it's possible because you can't introspect all possible context items. Perhaps I've overlooked some mechanism for doing so?

[wincent]

Perhaps I've overlooked some mechanism for doing so?

I guess by default you're only going to get the contextTypes that you registered for, and the only way around it would be to reach into internal React book-keeping data structures, which probably isn't going to happen...

[peterhorne]

Is there anything outstanding preventing this from being merged?

[wincent]

Is there anything outstanding preventing this from being merged?

I'd like to leave this one up for a bit to give people a chance to comment on it before we proceed. Three reasons why I'm inclined to be pretty conservative about this:

• It increases the (currently tiny) API surface area of Relay, something that we want to keep small (partly because it's much harder to revise or remove an API once it's out in the wild).
• Naming is hard: others might have opinions on pure as a name (my concerns: it is a very generic dictionary word, and it has substantial conceptual overlap with stateless functional components in React, yet saying pure: false doesn't necessarily mean that a component is impure in the the technical sense).
• That unresolved React thread (How to implement shouldComponentUpdate with this.context? react#2517) makes me wary that things may change in React context or alternative APIs/semantics may be introduced into React, and these in turn could make this new API at best temporary and at worst a liability for us.

So, let's gather a bit more input first.

[peterhorne]

Okay, thanks for the update 😃

The option name was originally disableShouldComponentUpdate: true and I changed it to pure: false following a suggestion on the original issue. On reflection I think the original name is better due to its explicitness.

[KyleAMathews]

I don't see the point of this tbh. If you really want this, just wrap your Relay component with https://github.com/reactjs/react-static-container

[peterhorne]

From react-static-container readme:

This component should be used when you know that a subtree of components will never need to be updated.

However, this PR introduces the ability to override Relay and enable updating of the subtree once again.

[josephsavona]

I agree that this is a valid use-case, but it also seems to be uncommon enough that it would be preferable to avoid increasing the API surface area, as @wincent mentioned.

Rather than add more API/options, I think we'll make more progress as a community if we focus on low-level primitives that can be composed together. What if it was so easy to build your own Relay Container that you could just do that and write your own shouldComponentUpdate?

[KyleAMathews]

That's exactly what react-static-container is designed for. It doesn't introduce any new ReactElements so you can add or remove it without triggering any DOM changes e.g.

if (SHOULD_DISABLE_UPDATES) {
  // Wrap Relay component.
  return <StaticContainer>{relayComponent}</StaticContainer>
} else {
  return relayComponent
}

[josephsavona]

I should clarify that i'm referring to #559.

[josephsavona]

@KyleAMathews the problem is that Relay will currently act as a static container and block updates, when the OP would prefer that it doesn't (bc data has changed on context).

[KyleAMathews]

@josephsavona ah :) didn't read the OP very closely I guess.

[taion]

Hi, sorry. This is partially my fault, but I forgot to subscribe to this PR.

I suggested pure: false because it mirrors the syntax for React Redux. It might not be the best name in a vacuum, but for better or for worse @gaearon has already staked out ground there, and I think it'd be less confusing overall to use a consistent name.

This is useful in the case where I'm using forms that communicate data via context, and I need to cause components to re-render based on contextual updates from the forms that would otherwise be blocked by the Relay containers.

The current userspace implementation is a bit gross:

import Relay from 'react-relay';

function shouldComponentUpdate() {
  return true;
}

export default function createFormInputContainer(...args) {
  const Container = Relay.createContainer(...args);

  function FormInputContainer(props, context) {
    const container = new Container(props, context);

    // Always update; this will be nested in a contained form anyway, and we
    // need to not block updates from the form on context.
    container.shouldComponentUpdate = shouldComponentUpdate;

    return container;
  }

  Object.keys(Container).forEach(key => {
    FormInputContainer[key] = Container[key];
  });

  return FormInputContainer;
}

This dance is necessarily of the lazy class initialization in Relay.createContainer.

[josephsavona]

I'm not crazy about increasing the surface area of RelayContainer, but the workaround (as documented by @taion) is not at all obvious. Let's move forward with this.

[josephsavona]

@facebook-github-bot import

[taion]

We actually just hit the opposite problem, BTW, in that there are a few places where we wish we could configure the Relay container would do shallow equality checks on non-scalar props the way the Redux container does. Probably too much to ask that this part of the code have yet another branch, though. 😉

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to Phabricator to review.

[chirag04]

It would be nice to have redux style shallow equality checks for non-scalars props. In react-native all my RelayContainer get a navigator object, Relay's SCU is not very helpful there.

[josephsavona]

@chirag04 Relay's shouldComponentUpdate is intended as a safe default. If you have complex objects (such that Relay's SCU always returns true), we recommend implementing shouldComponentUpdate in your component.

[taion]

Is there no downside to putting the SCU check both in the Relay container and in the contained component, though?

This came up in the context of the example F8 app, which uses a number of no-op Redux containers to make components be pure, like https://github.com/fbsamples/f8app/blob/b5df451259897d1838933f01ad4596784325c2ad/js/tabs/schedule/SharingSettingsModal.js#L91.

[josephsavona]

Is there no downside to putting the SCU check both in the Relay container and in the contained component, though?

The render logic of RelayContainer is very minimal - it basically just renders the component. Are you seeing a case where 2 SCUs is that much slower than one (compared to actually rendering the subtree)?

[taion]

Nope – I'm good for now, thanks!

[taion]

Out of curiosity, per my line note at 76477bb#commitcomment-17315176, what's the benefit of not allowing an actual shouldComponentUpdate implementation here? It looks like the API otherwise allows doing so.