Abort share fetches if the current user has changed #118
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add a `resetRequired` flag to override the current fetch request and reset if the current user has changed while the request is being made. If the component is not fetching, and has already been populated, reset as normal. This feels slightly clumsy, but corrects an issue where users were being shown the shares of other users, or concatenations of user shares because the `userId` was only updating when the fetch request was already in progress. Setting a `resetRequired` flag allows the results of the existing request to be ignored, and a new request made with the updated user details. Should fix this bug: https://trello.com/c/5v4YpJjr/452-first-time-going-in-on-a-user-who-does-not-have-any-pixel-kit-shares-you-see-other-peoples-pixel-kit-shares
murilopolese
reviewed
May 3, 2017
| // Abort the current request if a reset is required | ||
| if (this.resetRequired) { | ||
| this.reset(); | ||
| return; |
There was a problem hiding this comment.
Maybe we can use a more sophisticated way to cancel the fetch() request? whatwg/fetch#27
To be honest I feel we should cancel/abort/defer this request even before we get a result, no?
If we have a fetching property, wouldn't be nice to have the promise somewhere so we could just abort it and replace with something else?
There was a problem hiding this comment.
I've had a go at implementing cancelable promises in the PR for various other fixes: b547eff
This seems to fix the problem in a more elegant way, so I'll close this PR. We can always come back to this solution if things blow up in our faces.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a
resetRequiredflag to override the current fetch request and reset if the current user has changed while the request is being made. If the component is not fetching, and has already been populated, reset as normal. This feels slightly clumsy, but corrects an issue where users were being shown the shares of other users, or concatenations of user shares because theuserIdwas only updating when the fetch request was already in progress. Setting aresetRequiredflag allows the results of the existing request to be ignored, and a new request made with the updated user details.Should fix this bug: https://trello.com/c/5v4YpJjr/452-first-time-going-in-on-a-user-who-does-not-have-any-pixel-kit-shares-you-see-other-peoples-pixel-kit-shares