Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reset error when request succeeds #15

Merged
merged 1 commit into from
Jul 12, 2019
Merged

Reset error when request succeeds #15

merged 1 commit into from
Jul 12, 2019

Conversation

8enSmith
Copy link
Contributor

This is a fix for the issue "error is not reset if subsequent request succeeds".

Pretty sure this is the correct fix but please review!

@simoneb
Copy link
Owner

simoneb commented Jul 11, 2019

Thanks for this @8enSmith. I think this works fine but I'm not sure about the semantics. I can see two options, you have implemented the first:

  1. we reset the previous error as soon as we start the next request
  2. we reset the error only once we receive a successful response

I guess both would make sense, and I'm wondering if we should go for the first or the second, I think both have pros and cons.

@8enSmith
Copy link
Contributor Author

8enSmith commented Jul 12, 2019

@simoneb yes I agree that those are the two scenarios. However if a request fails and a refetch is triggered the previous error is redundant as it is only relevant to the previous failed request. When the refetch is triggered the error for that specific operation should be initialised to null as this is a new request.

Generally I think it is a better approach to keep updating the value of error after each request's response as opposed to keeping it set for historical failures.

That is my take on this anyhow!

Copy link
Owner

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

@8enSmith good argument. approved

@simoneb simoneb merged commit fe4b1fe into simoneb:master Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants