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

refactor(dev-middleware): drop node-fetch in favor of Node built-in fetch #45227

Conversation

byCedric
Copy link
Contributor

Summary:

Node 22 doesn't work well with node-fetch@2, as one of their polyfills is using the deprecated punycode module. This causes unnecessary warnings like:

image

Instead of upgrading to the much larger node-fetch@3, this change drops node-fetch in favor of Node's own built-in fetch implementation (using undici).

Note, @react-native/dev-middleware already has the engines.node >= 18 (which is required for fetch).

Changelog:

[GENERAL] [CHANGED] - Drop node-fetch in favor of Node's built-in fetch from undici in @react-native/dev-middleware

Test Plan:

See CI for passing tests

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 30, 2024
@byCedric byCedric changed the title refactor(dev-middleware): drop node-fetch in favor of Node fetch refactor(dev-middleware): drop node-fetch in favor of Node built-in fetch Jun 30, 2024
@facebook-github-bot facebook-github-bot added p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jun 30, 2024
@byCedric
Copy link
Contributor Author

Why would it fail on globalThis, it's the spec-guaranteed global 🙈

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 20,366,100 -4
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 23,562,903 -5
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: c7988c9
Branch: main

@facebook-github-bot
Copy link
Contributor

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

@NickGerleman
Copy link
Contributor

@byCedric if we use this with Node 18, or Node 20, do we get experimental API warnings?

@byCedric
Copy link
Contributor Author

byCedric commented Jul 3, 2024

@byCedric if we use this with Node 18, or Node 20, do we get experimental API warnings?

@NickGerleman No, there should not be any experimental warnings around fetch in the Node 18 or 20 (except versions older than Node 18.13.0 - released on Jan 6th, 2023):

Node 18.0.0 Node 18.20.3 Node 20.0.0
node-18.0.0 node-18.20.3 node-20.0.0

That warning was removed in Node 18.13.0, through PR 45287

@facebook-github-bot
Copy link
Contributor

@robhogan merged this pull request in 30a3e6e.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jul 3, 2024
Copy link

github-actions bot commented Jul 3, 2024

This pull request was successfully merged by @byCedric in 30a3e6e.

When will my fix make it into a release? | How to file a pick request?

@byCedric byCedric deleted the @bycedric/dev-middleware/drop-node-fetch branch July 3, 2024 16:38
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. p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants