Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

After a query has resulted in an error, variable updates that should result in an Apollo QueryResult with no errors do not update the QueryResult's error field in componentWillReceiveProps #1749

Closed
benjaminsaurusrex opened this issue Mar 2, 2018 · 8 comments
Labels
has-PR has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository

Comments

@benjaminsaurusrex
Copy link

benjaminsaurusrex commented Mar 2, 2018

Actual Result

  • Component mounts using graphql HOC, with a Query that produces an error
  • Update props so that variables passed to the operation options will now result in a query that succeeds with no errors
  • No network request is made, and inside componentWillReceiveProps, the nextProps.data.error is still defined with the previous error

Intended Result

  • Component mounts using graphql HOC, with a Query that produces an error
  • Update props so that variables passed to the operation options will now result in a query that succeeds with no errors
  • Either a network request is made, or the data is returned from the cache such that nextProps.data.error is no longer defined

How am I testing this?

Good question. I have a basic query that requests a field that always succeeds, and another field that fails when included:

// query.gql
query SomeQuery($shouldFail: Boolean!) {
  successField {
    id
  }
  failField @include(if: $shouldFail) {
    id
  }
}

Now for my components. I have 2 components, one that acts as a parent and determines whether its child shouldFail or not so that the child gets prop updates, which trigger a variable update when the operation options are different.

// Child
class ChildPresentation extends React.Component<Props> { ... }

// Child connected to graphql HOC
const Child = graphql(query, {
  options: (props: Props) => ({
    variables: {
      shouldFail: props.shouldFail,
    },
  }),
})(ChildPresentation);

// Parent
class Parent extends React.Component<ParentProps> {
  public render() {
    return <Child shouldFail={this.state.shouldFail} />;
  }
}

With this set up, you can see how Parent can easily update its state, which causes Child's shouldFail prop to update, which causes the variable inside the operation options to update... and down the rabbit hole.

The rabbit hole

When a GraphQL error comes through, the Observable decides to clean itself up.

https://github.com/zenparsing/zen-observable/blob/e924b1ddcde8aaac157cedb1908b2ad83b20e7bc/zen-observable.js#L184

Which removes it from ObservableQuery's list of observers (emptying it).

https://github.com/apollographql/apollo-client/blob/2.0/packages/apollo-client/src/core/ObservableQuery.ts#L538

Then when the new query variables come in, it follows the path of:

Which would normally end up doing the network request, but since earlier the observers list became empty, this ends up being a no-op.

https://github.com/apollographql/apollo-client/blob/2.0/packages/apollo-client/src/core/ObservableQuery.ts#L450

Then there's no recovery from react-apollo's perspective; when the graphql hoc calls subscribeToQuery at the end of componentWillReceiveProps (after the GraphQL.updateQuery above), that's a no-op because from its perspective it thinks it's still subscribed.

https://github.com/apollographql/react-apollo/blob/apollo-client-2.0/src/graphql.tsx#L432


The above was found and documented after some minor digging to see why this was happening.

Repro Environment

I actually don't have an easily accessible repro environment with public data that can be used for this. Though it should be fairly straightforward to set up by following my components above.

Version

@sjnonweb
Copy link

Having the exact same problem, except i'm using the Query component from react-apollo. @benjaminsaurusrex were you able to work around this issue?

According to this PR in apollo-client, the issue should be resolved but i'm still getting the same error.

These are the latest packages i'm using:

  • react-apollo: 2.1.9
  • apollo-client: 2.3.7

@guicar
Copy link

guicar commented Aug 31, 2018

@benjaminsaurusrex Have you tried using https://www.apollographql.com/docs/react/features/error-handling.html#policies with an error policy to all? This solved the problem of update of components with query errors in our case.

@ramivalta
Copy link

I recently ran into this (using Query from react-apollo also) and can confirm that setting errorPolicy to all indeed works around this bug, however on subsequent queries the failing query no longer returns an error, which seems a little surprising.

@rosskevin
Copy link
Contributor

Please consider submitting a PR with a unit test reproduction - that's the best way to get attention to this.

@MerzDaniel

This comment has been minimized.

@MerzDaniel
Copy link
Contributor

MerzDaniel commented Jan 9, 2019

Just created a PR with a test case.

What I observed in my local tests was:

  • first <Query>execution with correct variables works fine
  • as long changing the query variables still causes GraphQL errors
    - apollo sends out network request (as expected)
    - children comps of <Query> get updated with the new errror
    - BUT: the data in the props still contains the old data of the last successful execution
    (seems not to be intended for me, because the changed variables request completely different data)
  • if the variables change back to values of an already cached query result:
    - obviously no network request is sent out for the already cached data
    - BUT: the children of the <Query>component will not be updated (with the valid data as props)
  • changing variables to another set that is valid
    - <Query>is correctly executed and children are rendered with correct props

@MerzDaniel
Copy link
Contributor

MerzDaniel commented Jan 9, 2019

Tried out the errorPolicies:

  • The described usecase works fine somehow better with the policy ignore. It renders the children components in each case.
    BUT: the error is never passed down as renderProp so there is no way of determining why no data arrived
  • The policies none and all end up in causing the this bug.

So setting the policy to ignore is a workaround for now if you want the children to get updated after an error

@rosskevin rosskevin added has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository has-PR labels Jan 17, 2019
hwillson pushed a commit that referenced this issue Mar 15, 2019
* Add test for Query bug which is caused by changing props (#1749)

* Skip the failing test (#1749)

* Enable failing test

* Leverage lastResult to limit unnecessary subscription re-creation

When the Observable subscription created in `startQuerySubcription`
receives an error, we only want to remove the old subscription,
and start a new subscription, when we don't have a
previous result stored. So if no previous result was received due to
problems fetching data, or the previous result has been forcefully
cleared out for some reason, we shoule re-create a new
subscription. Otherwise we might be unnecessarily starting a new
subscription the `Query` component isn't expecting, and isn't tying
into its update cycle properly.

* Changelog update
@hwillson
Copy link
Member

This was fixed in #2718. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
has-PR has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository
Projects
None yet
Development

No branches or pull requests

7 participants