Allow disabling Relay container shouldComponentUpdate?

[taion]

I have a few use cases where I have components that receive data via React context. The current implementation of shouldComponentUpdate on Relay containers blocks updates to those components via context.

Would it make sense to allow disabling the optimized shouldComponentUpdate as a configuration option for Relay.createContainer? Right now I have to do something a bit ugly to get the behavior I want.

[devknoll]

This seems pretty reasonable. You should open a PR 😄

[taion]

I wanted someone to tell me if it seemed reasonable :p

Given facebook/react#2517 (comment), it looks like this will at most just be a temporary workaround – but it might be useful for some months.

[peterhorne]

I'd like to pick this up if no one else is currently working on it?

I've had a quick stab at it here: peterhorne@3e437df. If that looks alright please let me know and I shall add tests/docs and submit a PR.

[taion]

I never started working on this.

Your code is roughly what I had in mind. For consistency with e.g. React-Redux containers, it might make sense to make the setting be something like pure: false, though.

[peterhorne]

I've changed the setting name to pure as suggested and have submitted a PR #897

[taion]

This is inconvenient timing, but @gaearon has convinced me that shouldComponentUpdate blocking context updates is something that should be addressed by the library using context, not the library providing the container with shouldComponentUpdate: remix-run/react-router#470, remix-run/react-router#3430.

Specifically in the context of React Router, we're working on making our context use bypass intervening SCU checks by default, and extracting the utilities into a separate library for use by other context-using libraries: https://github.com/reactjs/react-router/pull/3430/files#diff-933e86e8d3d465b7c3d503e664607d59.

It's possible, then, that this is a bit of an anti-feature now, since I'm not aware of any other good reason to disable the Relay container pure render optimizations other than to allow context updates to propagate.

[josephsavona]

@taion thanks for the follow-up. We'd be happy to remove the sCU option, we'll probably wait until react-router and other popular libraries using context have updated with the approach you described.

[taion]

@josephsavona Our current tentative plan is to cut a React Router v3.0.0-pre.0 (or some other prerelease tag) within the next day or two, with the live links.

[bchenSyd]

@taion @devknoll @josephsavona the current replay container allows user to specify its own implementation of scu.
so if you pass a ()=>true, you effectively disabled RelayContainer SCU

what I'm interested in, is to integrate default SCU with customer provided SCU together. something like this:

shouldComponentUpdate(
      nextProps: Object,
      nextState: any,
      nextContext: any
    ): boolean {
  const  scu_result =   call existing SCU logic to get default result
  if(!scu_result && specShouldComponentUpdate){
      scu_result = specShouldComponentUpdate(this, nextProps, nextState, nextContext)
  }
 return  scu_result
}

there are 2 main changes in above code

1. if relay thinks it should re-render the component, then do it
2. if relay thinks it doesn't need to re-render the component, then asks the user's opinion
3. when calling customer provided SCU, please bind this and pass along props, state and context as the current implementation pass nothing

Please suggest if the idea makes sense and I can create a PR for it once confirmed.