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

fix: force update just after loading done with error even if the error is unchanged #3477

Merged

Conversation

jet2jet
Copy link
Contributor

@jet2jet jet2jet commented Sep 8, 2019

This PR will fix 'unchanging loading state' when an error, the same with previous error, is occurred after refetch (described in #3395).
# 'same' depends on @wry/equality implementation.

previousData.result.loading in QueryData should be true just before the first error handler called for any fetch/refetch, so this PR checks its value, and always calls forceUpdate if the value is true.
(related PR: #3339)

After:

  1. some fetch has triggered with notifyOnNetworkStatusChange: true
  2. loading changes to true
  3. an error occurrs for the fetch
  4. loading changes to false and the error data can be used
  5. call refetch
  6. loading changes to true
  7. an error occurrs for the refetch (the error message the same with the error occurred in step 3)
  8. loading changes to false and the error data can be used (this does not occur in the previous source)

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • If this was a change that affects the external API used in GitHunt-React, update GitHunt-React and post a link to the PR in the discussion.

@apollo-cla
Copy link

@jet2jet: 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/

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great @jet2jet - thanks very much!

@hwillson hwillson merged commit e9d33d8 into apollographql:master Sep 11, 2019
@jet2jet jet2jet deleted the fix-loading-on-refetch-same-error branch September 11, 2019 13:04
@gburchfield
Copy link

Not sure if this is the place to ask, but this issue is the closest to what I'm seeing when using 'useLazyQuery' and I see in the package it uses the 'QueryData.js' that was fixed in this PR.

I'm using @apollo/react-hooks@3.1.0

Experiencing the following:

const [login, {loading, error, data}] = useLazyQuery(LOGIN)

  1. Call login with bad credentials

  2. loading goes from true to false as expected

  3. trigger an Alert from the react-native package because error is not null/falsey for invalid authentication and click 'OK' to close message

  4. Call login again with bad credentials

  5. loading gets stuck on true

  6. GraphQL error is the same, as expected, Authentication Invalid

Expected Behavior:

  1. Call login with bad credentials

  2. loading goes from true to false

  3. Trigger Alert because of error then dismiss Alert

  4. Call login again with bad credentials

  5. error is cleared or nulled

  6. loading goes from true to false

  7. error is populated with a new Authentication Invalid error

Am I wrong to expect this behavior? Should I be using refetch? I tried to implement that but when calling refetch, loading was never changed to true. I also tried to clone the master branch this morning to get this merge in to test with my project but I couldn't get it implemented correctly and have just relied on the npm install packages of botho 3.1.0 and 3.1.0.beta.0. For now my solution will just be to handle loading and error locally with useState.

@dylanwulf
Copy link
Contributor

@gburchfield this fix will be in the v3.1.1 release. If you still have the problem when v3.1.1 is released, please open a new issue

@gburchfield
Copy link

Updated my @apollo/react-hooks package via npm to v3.1.1

I am still seeing the problem I described above:

@#3477 (comment)

Also the issue could be similar to #3468

My workaround is to just navigate to an entirely new component/screen, ensuring the component I'm using gets unmounted and then navigate back to my component fresh. Probably the best for a login screen, but for other use cases I could see this being really annoying, especially if the work arounds I'm reading include using a forceUpdate()

Either way, great library so far! As Apollo matures, this will be my preference for both mobile and web. Thanks guys!

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.

None yet

5 participants