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

Fix problem with calling the browser version concurrently #53

Merged
merged 11 commits into from
Dec 7, 2020

Conversation

stropitek
Copy link
Contributor

@stropitek stropitek commented Oct 31, 2020

  • Uses closure on xhr to avoid referencing the wrong xhr object in the load callback if multiple requests are done in parallel
  • Remove the abort method that was not exported so was useless

Closes #52

@sindresorhus
Copy link
Owner

Remove the abort method that was not exported so was useless

That's just a mistake. It should be exported. And it is documented to work like that. Can you properly export it (add it to those exported methods)?

@sindresorhus sindresorhus changed the title fix: use closure on xhr Fix problem with calling the browser version concurrently Oct 31, 2020
@stropitek
Copy link
Contributor Author

I was not sure what to do with the promise if the request gets canceled. In this implementation, the promise resolves with undefined as soon as the user calls cancel

@stropitek
Copy link
Contributor Author

@sindresorhus anything I can do to get this landed?

I tested this implementation here. When cancelled, it aborts the current request and prevents the execution of the remaining fallback urls. And it resolves with undefined to avoid memory leaks (hanging promise that never resolves)

@sindresorhus
Copy link
Owner

And it resolves with undefined to avoid memory leaks (hanging promise that never resolves)

This violates the types and docs though as it's documented to always return a string if it doesn't error.

@sindresorhus
Copy link
Owner

The Node.js version throws a CancelError. I think the browser version should do the same.

https://github.com/sindresorhus/p-cancelable/blob/5e8b26853d994c1de14232ff839945fc331d2912/index.js#L3-L12

@stropitek
Copy link
Contributor Author

@sindresorhus
Copy link
Owner

Always run $ npm test locally before pushing.

@sindresorhus sindresorhus merged commit f0a7f07 into sindresorhus:master Dec 7, 2020
@sindresorhus
Copy link
Owner

Thank you :)

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.

Calling twice in parallel can lead to promise rejection
2 participants