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

Add test for Query bug which is caused by changing props (#1749) #2718

Merged
merged 13 commits into from
Mar 15, 2019

Conversation

MerzDaniel
Copy link
Contributor

The bug was reported some months ago and I stumbled over it after 1h of trying out react-apollo. This test show-cases the wrong behaviour and fails.

In the related bug ticket someone already debugged into the cause of this issue.

#1749

@apollo-cla
Copy link

@MerzDaniel: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@hwillson
Copy link
Member

Thanks for this @MerzDaniel - something definitely appears to be off in case 2. I've re-enabled the test so this PR will now fail, and will dig into this further. One thing to note however - the expect(props.data.allPeople).toBeFalsy() check you had in case 1 is invalid. React Apollo makes the last known good data available when an error is received, in-case your UI wants to leverage the good data in some way. I've adjusted your test accordingly.

@hwillson hwillson self-assigned this Mar 11, 2019
@hwillson
Copy link
Member

Noting this here in-case I'm sidetracked and someone else is interested in taking a look - this test is currently failing because of the query re-subscription logic that happens when the error is received:

this.resubscribeToQuery();

The resubscribeToQuery approach is a known hack (see the discussion in #1580). This should be addressed in apollo-client proper.

@sfcgeorge
Copy link

Not sure if this is the same bug but I have had issues with changing props not updating the UI properly.

I think the first query made and cached with the first set of props causes issues. Subsequent changes to props and thus queries render fine, but going back to the initial set of props doesn't update the UI.

I tried to reproduce using CodeSandbox but couldn't, so I think it needs a real network layer to occur.

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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants