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

Remove vendored fetch polyfill, update to whatwg-fetch@3.0 #24418

Closed
wants to merge 2 commits into from

Conversation

@janicduplessis
Copy link
Contributor

janicduplessis commented Apr 11, 2019

Summary

The original reason for vendoring the fetch polyfill was to remove the default blob response type but this was reverted.

Here's a little history around the fetch polyfill and the blob issue:

  • Original commit introducing the vendored polyfill: #19333, the goal was to fix a memory leak because our blob implementation doesn't release resources automatically. Not an ideal fix but since the issue was pretty severe and the infra for a proper fix was not in place.
  • This introduced an issue when downloading images using fetch which was fixed by #22063 which re-added the default blob content type. However that re-introduced the original fetch memory leak.
  • We have better infra now with jsi and I was able to get blob deallocation working, see #24405

Currently the vendored fetch polyfill is useless since it was changed back to the original version. We can just use the npm version again. I also updated to 3.0 which brings better spec compliance and support for cancellation via AbortController, https://github.com/github/fetch/releases/tag/v3.0.0.

Changelog

[General] [Changed] - Remove vendored fetch polyfill, update to whatwg-fetch@3.0

Test Plan

Tested using fetch in RN tester to make sure everything still works.

@@ -21,6 +21,10 @@ if (global.window === undefined) {
global.window = global;
}

if (global.self === undefined) {

This comment has been minimized.

Copy link
@janicduplessis

janicduplessis Apr 11, 2019

Author Contributor

whatwg-fetch@3 relies on the self global

This comment has been minimized.

Copy link
@TheSavior

TheSavior Apr 11, 2019

Member

Maybe worth putting this comment in the code?

This comment has been minimized.

Copy link
@janicduplessis

janicduplessis Apr 11, 2019

Author Contributor

I didn’t since it’s not specific to the fetch polyfill and is just a standard dom api like Window (https://developer.mozilla.org/en-US/docs/Web/API/Window/self). I don’t mind adding it though if you prefer.

@pull-bot

This comment has been minimized.

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 ab92cb5

@cpojer

This comment has been minimized.

Copy link
Contributor

cpojer commented Apr 12, 2019

Yay, thank you for doing this and removing code that we have to maintain! I'm going to wait until we merge the blob memory leak PR before landing this one.

@cpojer cpojer mentioned this pull request Apr 12, 2019
41 of 64 tasks complete
@janicduplessis

This comment has been minimized.

Copy link
Contributor Author

janicduplessis commented Apr 12, 2019

@cpojer We don't actually have to wait for the blob memory leak PR to land, the leak happens with or without this. That's fine if you prefer to wait though :)

@cpojer

This comment has been minimized.

Copy link
Contributor

cpojer commented Apr 12, 2019

Oh good to know, I'll get to it soon then :)

@cpojer
cpojer approved these changes Apr 15, 2019
Copy link
Contributor

cpojer left a comment

This diff makes me so happy!

Copy link

facebook-github-bot left a comment

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

facebook-github-bot added a commit that referenced this pull request Apr 15, 2019
Summary:
This adds https://github.com/mysticatea/abort-controller to polyfill [AbortController](https://developer.mozilla.org/en-US/docs/Web/API/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

[General] [Added] - Add support for cancelling fetch requests with AbortController
Pull Request resolved: #24419

Differential Revision: D14912858

Pulled By: cpojer

fbshipit-source-id: 8a6402910398db51e2f3e3262f07aabdf68fcf72
@janicduplessis

This comment has been minimized.

Copy link
Contributor Author

janicduplessis commented Apr 18, 2019

@cpojer Fixed a conflict, wanna try landing this again?

Copy link

facebook-github-bot left a comment

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

@zhongwuzw

This comment has been minimized.

Copy link
Collaborator

zhongwuzw commented May 7, 2019

Ahha, after mentioned by @zhigang1992, I found fetch is to be managed by npm directly, I created a PR #24726 to fix fetch withCredentials issue, that is because security policy is different between web and iOS/Android, detailed discussion can see #14063. I don't know wether we have another differences. Any opinion about this?

@hramos

This comment has been minimized.

Copy link
Contributor

hramos commented May 8, 2019

This might take a while longer to land. Targeting next week, at the earliest.

@mislav

This comment has been minimized.

Copy link

mislav commented May 13, 2019

Hello, I'm one of the authors and maintainers of whatwg-fetch 👋

I'm happy to learn that adding self to React Native will make this problem go away github/fetch#657

I have also found this puzzling comment:

// RN currently lazy loads whatwg-fetch using a custom fetch module, which,
// when called for the first time, requires and re-exports 'whatwg-fetch'.
// However, when a dependency of the project tries to require whatwg-fetch
// either directly or indirectly, whatwg-fetch is required before
// RN can lazy load whatwg-fetch. As whatwg-fetch checks
// for a fetch polyfill before loading, it will in turn try to load
// RN's fetch module, which immediately tries to import whatwg-fetch AGAIN.
// This causes a circular require which results in RN's fetch module
// exporting fetch as 'undefined'.
// The fix below postpones trying to load fetch until the first call to symbolicateStackTrace.
// At that time, we will have either global.fetch (whatwg-fetch) or RN's fetch.
if (!fetch) {
fetch = global.fetch || require('../../Network/fetch').fetch;

Can anyone explain is this still relevant? Is there something we can tweak in whatwg-fetch to make this workaround obsolete?

Finally, what is the reason React Native bundles a window.fetch polyfill in the first place? Are any of the browser targets for React Native apps such old browsers that have no native support for window.fetch?

Thank you!

@janicduplessis

This comment has been minimized.

Copy link
Contributor Author

janicduplessis commented May 13, 2019

Hi @mislav!

I don't have context on the comment so I can't help there.

We use the fetch polyfill because React Native does not run in a browser environment, it runs in JavaScriptCore (or possibly another JS engine) so it doesn't have any DOM apis built in. To avoid having to write a native implementation for both XMLHttpRequest and fetch we have one for XMLHttpRequest only and use a fetch polyfill. So this means the fetch polyfill will always be used in React Native.

Copy link

facebook-github-bot left a comment

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

@react-native-bot

This comment has been minimized.

Copy link
Collaborator

react-native-bot commented May 31, 2019

This pull request was successfully merged by @janicduplessis in bccc92d.

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

facebook-github-bot added a commit that referenced this pull request Jun 6, 2019
Summary:
The old whatwg-fetch module doesn't actually export anything, so we would always hit the `else` condition.

The new whatwg-fetch (3.0.0, introduced in #24418) now exports an ES module. As a result, `whatwg` and `whatwg.fetch` are both truthy but the `module.exports` will end up empty. This breaks the RN fetch module.

This will switch the behavior back to the expected polyfill behavior (calling `require('whatwg-fetch')` and allowing it to polyfill fetch in global scope). The RN fetch module will re-export these globals.

Reviewed By: cpojer

Differential Revision: D15639851

fbshipit-source-id: ebd8bce85f7797d8539f53982e515ac47f6425e7
kelset added a commit that referenced this pull request Jun 7, 2019
Summary:
The original reason for vendoring the fetch polyfill was to remove the default blob response type but this was reverted.

Here's a little history around the fetch polyfill and the blob issue:

- Original commit introducing the vendored polyfill: #19333, the goal was to fix a memory leak because our blob implementation doesn't release resources automatically. Not an ideal fix but since the issue was pretty severe and the infra for a proper fix was not in place.
- This introduced an issue when downloading images using `fetch` which was fixed by #22063 which re-added the default blob content type. However that re-introduced the original fetch memory leak.
- We have better infra now with jsi and I was able to get blob deallocation working, see #24405

Currently the vendored fetch polyfill is useless since it was changed back to the original version. We can just use the npm version again. I also updated to 3.0 which brings better spec compliance and support for cancellation via `AbortController`, https://github.com/github/fetch/releases/tag/v3.0.0.

## Changelog

[General] [Changed] - Remove vendored fetch polyfill, update to whatwg-fetch@3.0
Pull Request resolved: #24418

Differential Revision: D14932683

Pulled By: cpojer

fbshipit-source-id: 915e3d25978e8b9d7507ed807e7fba45aa88385a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.