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

Add support for cancelling fetch requests with AbortController #24419

Closed

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Apr 11, 2019

Summary

This adds https://github.com/mysticatea/abort-controller to polyfill AbortController. This is used to cancel requests when using fetch.

This also updates event-target-shim to 5.0 to make sure we only have one version of this dependency. This updates required adding a polyfill for console.assert which is used by the new version. I made one based on https://github.com/gskinner/console-polyfill/blob/master/console.js#L74.

The polyfill is very small, especially since we already use event-target-shim so I think it makes sense to include in core.

Depends on #24418 so that the fetch polyfill supports the signal parameter.

Fixes #18115

Changelog

[General] [Added] - Add support for cancelling fetch requests with AbortController

Test Plan

Added a RNTester example of cancelling fetch requests.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Expo Partner: Expo Partner labels Apr 11, 2019
@pull-bot
Copy link

pull-bot commented Apr 11, 2019

Warnings
⚠️ 🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against a38e50c

@react-native-bot react-native-bot added 🌐Networking Related to a networking API. Includes Changelog labels Apr 11, 2019
@matthargett
Copy link
Contributor

once this (and its prerequisite PR) is merged, I'd like to propose it for backport to 0.59. @dysonpro fixed this in our apps by injecting/overriding RN's fetch with yetch (thanks @jayphelps !) and some other bits of magic.

we'd just need to make sure this doesn't introduce the same memory leak issues like the last time this abort controller support was attempted.

@janicduplessis
Copy link
Contributor Author

@matthargett What memory leak are you talking about? Is it the one I described in #24418 or something else I should be aware of?

@cpojer
Copy link
Contributor

cpojer commented Apr 12, 2019

@matthargett we aren't going to backport new features to 0.59, let's keep this work focused on 0.60+.

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

@janicduplessis thank you so much, this is so good!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @janicduplessis in 5e36b0c.

When will my fix make it into a release? | Upcoming Releases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. 🌐Networking Related to a networking API. p: Expo Partner: Expo Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AbortController is missing
6 participants