Remove vendored fetch polyfill, update to [email protected]

[janicduplessis]

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: Vendor fetch polyfill, remove default blob response type #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 set blob as default XMLHttpRequest header response type if supported #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 [Blob] Release underlying resources when JS instance in GC'ed on iOS #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 [email protected]

Test Plan

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

[janicduplessis]

whatwg-fetch@3 relies on the self global

[elicwhite]

Maybe worth putting this comment in the code?

[janicduplessis]

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]

	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]

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.

[janicduplessis]

@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]

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

[cpojer]

This diff makes me so happy!

[facebook-github-bot]

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

[janicduplessis]

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

[facebook-github-bot]

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

[zhongwuzw]

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 might take a while longer to land. Targeting next week, at the earliest.

[mislav]

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 JakeChampion/fetch#657

I have also found this puzzling comment:

react-native/Libraries/Core/Devtools/symbolicateStackTrace.js

Lines 29 to 41 in ddce743

	// 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]

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.

[facebook-github-bot]

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

[react-native-bot]

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

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