Relay: 0.8.0 setVariables change difficult when variables not yet updated

[fionawhim]

I'm running into trouble due to this change:

“RelayContainer.setVariables will no longer check if the variables are changed before re-running the variables. To prevent extra work, check the current variables before calling setVariables.”

My problem is that my code had previously been doing a setVariables call from componentWillReceiveProps, which seems to (at least in my case) be called as a result of setVariables.

This is leading to an infinite loop, but I can’t use the suggestion to check the current value of the variable because it isn’t updated until the variable fetch succeeds.

While I can be more defensive about not considering setVariables based on what props are changed, I worry that this is more fragile.

Can you restore the previous setVariables behavior, or make it easier to see what variables values are in-flight so I can make my own check more accurate?

[josephsavona]

Thanks for letting us know about this issue. In general, calling setVariables in componentWillReceiveProps can lead to an infinite loop. Changing variables fetches more data, which will typically cause the component to re-render, which will cause new props to be received. I'll get to a workaround/follow-up in a moment, but before that - what's the use-case for setting variables in willReceiveProps? If a component needs data, that _should_ be expressible in its fragments such that the parent can ensure all data is available.

We encountered some issue with the old behavior of setVariables and don't plan on reverting to it. Specifically, it's possible for one component in a to call forceFetch such that another component ends up missing some data (forceFetch on a range overwrites the range - if i force fetch for 5 items and you had fetched 20, now you'll suddenly get only 5. we refer to this as the "missing records/items problem"). The only option before the setVars change was for the affected component to also forceFetch (because setVars would previously see that the affected components vars were the same and avoid refetching), which could cause an infinite loop of refetching. The new behavior allows a component so affected to use setVars and avoid the infinite loop (because setVars merges new range items instead of overwriting).

tl;dr Yes, we should make it possible to see what vars are pending, but I'm still curious about the use-case for calling setVariables in willReceiveProps (seems like an anti-pattern).

cc @yuzhi

[josephsavona]

I've filed a new issue at #1049 to describe the addition of pendingVariables.

[wincent]

Also curious to know what about your use case requires you to use componentWillReceiveProps (as opposed to, say, componentDidUpdate).

[fionawhim]

Thanks for the info about the problems with the previous implementation! Here's what we're up to, and maybe you'll have a suggestion for a better approach.

We're rendering a list of elements with a filtering search box. We're doing the filtering client-side, so we want to load the complete list into memory (up to at least a big max of 5000 elements or so). For keeping the individual requests reasonable, though (we have resource limits on the server to mitigate abuse), we're making requests for 100 or so at a time.

So, the logic is that we have a "count" variable that feeds into the "first" argument of the elements' connection. When props update, we check the connection to see if it has a next page. If so, we set the variable to 100 + the current number of edges to fetch the next page.

We'd probably be happier either doing this in parallel, or always setting "first" to the 5000 max, with some combination of server-side limit enforcement and Relay automatically making follow-up requests to get more pages.

It might also be that componentDidUpdate would work? We don't actually render all of the loaded elements by default; they're just in memory for the filtering.

[josephsavona]

@finneganh I see, thanks for clarifying. In general we recommend doing this type of fetching in componendDidMount. You might set things up as:

componentDidMount() {
  // begin loading more only once the component has mounted
  this.fetchMore();
}
fetchMore() {
  if (!this.props.results.pageInfo.hasNextPage) {
    return; // nothing more to fetch
  }
  const {count} = this.props.relay.variables;
  this.props.relay.setVariables({count: count + 100}, {done} => {
    if (done) {
      // continue loading when the previous fetch completes
      this.fetchMore();
    }
  });
}

[fionawhim]

Ah, that looks tons cleaner. Thanks for the tip, I'll use that approach.

Closing this ticket because you're tracking pendingVariables elsewhere.

[josephsavona]

@finneganh Glad that helps, thanks for the follow-up!

[jardakotesovec]

I am having similar problem. My use case is if I get new props from parent I need to adjust relay variables accordingly . So I thought that componentWillRecieveProps or componentDidUpdate would be good place to do that.

But issue is with the infinite loop (with both 0.7.3 and 0.8) - seems that component gets updated immediately after calling setVariables, which causes next setVariable call.. and it does not wait for the actual requests, just generate as much requests as it can in infinite loop.

I am not able to reproduce it in relay playground so I am still in process figuring out whats going on. Just in case someone has insight and/or dealing with this as well...

[jardakotesovec]

So here is playground.

To achieve infinite loop in this playground - only thing that needed is to change breakMe={5} to something non-scalar like breakMe={{}}. As result shouldComponentUpdate in RelayContainer always returns true which is the issue because setState get called on container immediately after setVariable.

So I guess solution here is to implement proper shouldComponentUpdate in component itself and call setVariables in componentDidUpdate? And in 0.8 I should also make the comparison with current variables (which will be possible once we have pending variable prop) before I call new setVariables?

Currently feel overwhelmed to do relatively simple thing. Am I doing it wrong?

[josephsavona]

@jardakotesovec Thanks for providing more detail here. This came up again in #1063 (see my comment). It sounds like we need to adjust RelayContainer such that it's safe to call setVariables from within the child's componentWillReceiveProps call.

We'll look at this as time permits, but are definitely open to PRs to help here.

[mickeyinfoshan]

@jardakotesovec I think you can check your props before setVariables is called in componentWillReceiveProps to break the loop. For example:

componentWillReceiveProps(nextProps){
    if (this.props.foo !== nextProps.foo){
        this.props.relay.setVariables({...})
    } 
}

[jardakotesovec]

@mjhlybmwq yes.. or implement shouldComponentUpdate and call setVariables in componentDidUpdate.

[fionawhim]

@josephsavona Just wanted to drop in our solution here to show what we ended up going with, in case it might be informative for future Relay features: https://gist.github.com/finneganh/c81bd1167a07306eba7365f8e123ccfb

One note about the suggestion that you put above is that at the time of the setVariables callback’s ready state being “done,” the component’s props haven’t been updated and so fetchMore wouldn’t see the right view of the world.

We ended up putting things through componentDidUpdate, with some checks to both prevent double-fetching and also a backoff if the fetches are failing or otherwise not making progress (so as not to fetch on an infinite loop, especially during prod issues).

One curiosity we had was that for our prod component, componentWillReceiveProps was consistently called (twice actually) after the setVariables call errored out. In a smaller test component, it wasn’t called. I don’t know if there’s an explanation for that behavior (the prod component had sub-containers, maybe that was it?) but we ended up working around it with the forceUpdate.

At any rate, I found this an interesting case where we seemed to be running at odds with how Relay wants to do data access. I don’t know if that means we’re doing something particularly wrong, or just running into the law of leaky abstractions.