Skip to content

Conversation

dario-piotrowicz
Copy link
Contributor

currently in our bundle we include the next/dist/compiled/node-fetch package, this doesn't seem actually necessary and it introduces a good amount of unnecessary code in our bundle, moreover it also causes a warning in the terminal (because of an === -0 comparison), the changes here replace the node-fetch code with a simply shim that re-exports the native fetch


Notes:

  • by looking at the Next.js source code I wouldn't really tell why node-fetch is used, I can only imagine that it's there for either backward compatibility or brought in as part of some dependencies
  • all the Next.js source code that would import from node-fetch that I've found always only imports just the default export
  • I couldn't find a way to trigger a Next.js app to call node-fetch (as far as I can tell it's generally unused)
  • I think this change if beneficial for the two reasons I mentioned above (less code in our bundle + removed warning) but it introduces a small risk (assuming that node-fetch would actually work in workerd), I think the risk is very tolerable especially since all the e2es pass

If someone disagree with this PR I'm ok changing course and just patch the === -0 check (although I do think it is pretty nice to reduce the bundle size here as an added bonus)


resolves #321

currently in our bundle we include the `next/dist/compiled/node-fetch` package,
this doesn't seem actually necessary and it introduces a good amount of unnecessary
code in our bundle, moreover it also causes a warning in the terminal (because of an
=== -0 comparison), the changes here replace the node-fetch code with a simply shim
that re-exports the native fetch
Copy link

changeset-bot bot commented Feb 27, 2025

⚠️ No Changeset found

Latest commit: 4767aeb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

pkg-pr-new bot commented Feb 27, 2025

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/cloudflare@422

commit: 4767aeb

Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

Nice!

@dario-piotrowicz dario-piotrowicz merged commit a7bf738 into main Feb 27, 2025
7 checks passed
@dario-piotrowicz dario-piotrowicz deleted the dario/node-fetch-shim branch February 27, 2025 15:06
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.

[BUG] handler.mjs: Comparison with -0 using the "===" operator will also match 0 [equals-negative-zero]

2 participants