React-apollo: Inifinite calling of onError in query hoc

[stefan-krajnik]

Intended outcome:
I expect onError to be called only once when a response is received.

Actual outcome:
I have a mutation (@client) which I call in onError, but the mutation seems to be triggering some process in which onError (with same error from the request call) is called again

How to reproduce the issue:
Here is the block of code I'm using.
_Note: if I don't call createNotification everything works as expected_

export default compose(
  graphql(createNotificationMutation, { name: 'createNotification' }),
  graphql(someQuery, {
    name: 'someQuery',
    options: ({ createNotification }) => ({
      variables: {
        // ....
      },
      notifyOnNetworkStatusChange: true,
      onError: (err) => {
        createNotification({
          variables: {
            data: {
              text: err.message,
              isError: true,
            },
          },
        })
      },
    }),
  }),
)(MyComponent)

Version

• apollo-client@ - I don't have direct apollo-client dependency
• [email protected]
• [email protected]

[manurg]

Hey, we are struggling as well with an infinite loop caused by probably this PR https://github.com/apollographql/react-apollo/pull/2190 @olistic

We are showing query errors via react context.

Pseudo code implementation:

// container having MyComponent as children
class ErrorContainer extends React.Component {
  constructor(props) {
    super(props);

    this.showError = error => {
      this.setState(() => ({error}));
    };

    this.state = {
      error: null,
      showError: this.showError
    };
  }

  render() {
    const { error } = this.state;
    const { children } = this.props;

    return (
      <ErrorContext.Provider value={this.state}>
        {children}
      </ErrorContext.Provider>
    );
  }
}

// MyComponent having the Query
<ErrorContext.Consumer>
    {({ showError }) => (
      <Query
        query={MyQuery}
        onError={showError}
      />
  )}
</ErrorContext.Consumer>

I am not sure at this point if the PR just showed us some hidden problems we did not see in our app yet or if this introduced some bugs, as implementation in componentDidUpdate can easily lead to infinite loops 🤔

[olistic]

I am not sure at this point if the PR just showed us some hidden problems we did not see in our app yet

@manurg Either that or the way we were doing things in our code became problematic with this update.

In my scenario, I was calling setState() from the onCompleted() callback without wrapping it in a condition. Because onCompleted() is now run in the context of componentDidUpdate(), I had to adapt my code to follow this rule, from the official docs:

You may call setState() immediately in componentDidUpdate() but note that it must be wrapped in a condition like in the example above, or you’ll cause an infinite loop.

Maybe @hwillson can comment a bit more on this issue.

[manurg]

@olistic @stefan-krajnik
I think this problem was implemented with https://github.com/apollographql/react-apollo/pull/2190
If you call setState within onError handler (which now also gets called within Querys componentDidUpdate, you create an infinite loop. IMHO this is unexpected.

I created a little sandbox:

This is the preview in the browser:
https://k04rq99845.codesandbox.io/

You can force an error by blocking the call to w5xlvm3vzz.lp.gql.zone/graphql and hit reload

[olistic]

@manurg You're right, it's unexpected. As @stefan-krajnik has said, one could expect the onError() (and onCompleted()) callbacks to be called only once (when the query errors out or when it succeeds, respectively).

2190 was a fix for the onCompleted() callback not being called when the data was already in the Apollo Cache. If that fix required moving the logic that calls onCompleted() and onError() to the componentDidUpdate() method of the Query component, maybe that should've been wrapped in a condition to only call it once? (And not the code we execute in those callbacks.) Just thinking out loud here, I'd wait for @hwillson's comments on this.

[hwillson]

Thanks for looking into this everyone. @olistic, your suggested fix sounds reasonable to me. We'll definitely review a PR if anyone is interested. Thanks!

[chenesan]

@hwillson I'd love to work on this :)

[juddey]

Hey there @chenesan I started to work on this but if you want to use what I have done to put a pr together then feel free to borrow my (very rough) code:

Test:

    it('onCompleted should allow setState in callback', done => {
      const query = gql`
        query people($first: Int) {
          allPeople(first: $first) {
            people {
              name
            }
          }
        }
      `;

      const data1 = { allPeople: { people: [{ name: 'Luke Skywalker' }] } };
      const data2 = { allPeople: { people: [{ name: 'Han Solo' }] } };
      const mocks = [
        {
          request: { query, variables: { first: 1 } },
          result: { data: data1 },
        },
        {
          request: { query, variables: { first: 2 } },
          result: { data: data2 },
        },
      ];

      let count = 0;

      class Component extends React.Component {
        state = {
          sausages: true,
          variables: {
            first: 1,
          },
        };

        componentDidMount() {
          setTimeout(() => {
            this.setState({
              variables: {
                first: 2,
              },
            });
            setTimeout(() => {
              this.setState({
                variables: {
                  first: 1,
                },
              });
            }, 50);
          }, 50);
        }

        // Make sure `onCompleted` is called both when new data is being
        // fetched over the network, and when data is coming back from
        // the cache.

        render() {
          const { variables } = this.state;

          return (
            <AllPeopleQuery query={query} variables={variables} onCompleted={() => {
              // console.log("I'm done.")
              this.setState({ sausages: true })
              done()
            }}>
              {() => null}
            </AllPeopleQuery>
          );
        }
      }

      wrapper = mount(
        <MockedProvider mocks={mocks} addTypename={false}>
          <Component />
        </MockedProvider>,
      );
    });
  });

Fix so far:

  componentDidUpdate(prevProps: any) {
    const { onCompleted, onError } = this.props;
    if (onCompleted || onError) {
      const currentResult = this.queryObservable!.currentResult();
      const { loading, error, data } = currentResult;
      if (onCompleted && !loading && !error) {
        if (this.props.onCompleted === prevProps.onCompleted) {
          onCompleted(data)
        }
      } else if (onError && !loading && error) {
        onError(error);
      }
    }
  }

The test passes, I just haven't got to editing nicely PR'ing yet, also the change also needs to be implemented for the onError callback.

[chenesan]

Thanks a lot for your work @juddey ! I'll try this. should be able to send a PR in this week.