-
Notifications
You must be signed in to change notification settings - Fork 8
feat(fetch): use native global fetch instead of cross-fetch for workers #777
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| // the dist assertion requires to run `npm run build` before running the test | ||
|
|
||
| import { readFileSync } from 'fs'; | ||
| import polyfillFetch from './fetch-polyfill'; | ||
|
|
||
| describe('fetch-polyfill', () => { | ||
| it('should delegate to the runtime native fetch', async () => { | ||
| const res = { ok: true }; | ||
| const spy = jest.spyOn(globalThis, 'fetch').mockResolvedValue(res as Response); | ||
|
|
||
| await expect(polyfillFetch('https://example.com', { method: 'POST' })).resolves.toBe(res); | ||
| expect(spy).toHaveBeenCalledWith('https://example.com', { method: 'POST' }); | ||
| }); | ||
|
|
||
| it('should not bundle node-only http clients into the build', () => { | ||
| const dist = readFileSync('./dist/index.esm.js', 'utf-8'); | ||
| ['cross-fetch', 'node-fetch', 'node:http', 'node:https'].forEach((specifier) => { | ||
| expect(dist).not.toContain(specifier); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,31 +1,7 @@ | ||
| import { fetch as crossFetch, Headers } from 'cross-fetch'; | ||
|
|
||
| globalThis.Headers ??= Headers; | ||
|
|
||
| const highWaterMarkMb = 1024 * 1024 * 30; // 30MB | ||
|
|
||
| // we are increasing the response buffer size due to an issue where node-fetch hangs when response is too big | ||
| const patchedFetch = (...args: Parameters<typeof crossFetch>) => { | ||
| // we can get Request on the first arg, or RequestInfo on the second arg | ||
| // we want to make sure we are setting the "highWaterMark" so we are doing it on both args | ||
| args.forEach((arg) => { | ||
| // Updated to only apply highWaterMark to objects, as it can't be applied to strings (it breaks it) | ||
| if (arg && typeof arg === 'object') { | ||
| // eslint-disable-next-line no-param-reassign, @typescript-eslint/no-unused-expressions | ||
| (arg as any).highWaterMark ??= highWaterMarkMb; | ||
| } | ||
| }); | ||
|
|
||
| return crossFetch(...args); | ||
| }; | ||
|
|
||
| // node-fetch@2 (bundled by cross-fetch) throws a false ERR_STREAM_PREMATURE_CLOSE on | ||
| // keep-alive responses on Node >= 22.23.0 / 24.17.0 (nodejs/node#63989, the CVE-2026-48931 | ||
| // http.Agent fix). Node's built-in fetch (undici, Node >= 18) is unaffected, so prefer it | ||
| // when present and fall back to cross-fetch (node-fetch) only on older runtimes. | ||
| const polyfillFetch = | ||
| typeof globalThis.fetch === 'function' | ||
| ? (...args: Parameters<typeof globalThis.fetch>) => globalThis.fetch(...args) | ||
| : patchedFetch; | ||
| // Native fetch only (Node >= 18, browsers, Cloudflare Workers and other edge runtimes). | ||
| // Bundling a Node-based polyfill (cross-fetch/node-fetch) pulls `http`/`https` into edge | ||
| // builds, where unenv stubs them with functions that throw on call. | ||
| // Bound through a wrapper so undici's fetch keeps its correct `this`. | ||
| const polyfillFetch = (...args: Parameters<typeof globalThis.fetch>) => globalThis.fetch(...args); | ||
|
|
||
| export default polyfillFetch as unknown as typeof fetch; | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | |||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -25,7 +25,7 @@ | ||||||||||||||||
| "url": "git://github.com/descope/node-sdk.git" | |||||||||||||||||
| }, | |||||||||||||||||
| "engines": { | |||||||||||||||||
| "node": ">= 16.0.0" | |||||||||||||||||
| "node": ">= 18.0.0" | |||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MEDIUM: This is a breaking change for Node 16 consumers, but the PR is titled
Node 16 is EOL so dropping it is reasonable, but consider a major (or at minimum an explicit release-note callout).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid, and @asafshen agreed — this is now a major. That closes the escape hatch you identified: on a major, a Node 16 consumer on
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this implies we need to bump major
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed — going major. Pushed One thing you need to do, though: this repo squash-merges (every commit on
The Who's on node 16/17: Node 16 EOL'd Sept 2023, Node 17 June 2022. CI was still matrixing 16 (dropped in this PR), but that was the only thing keeping it alive. With a major, |
|||||||||||||||||
| }, | |||||||||||||||||
| "scripts": { | |||||||||||||||||
| "build": "rimraf dist && rollup -c", | |||||||||||||||||
|
|
@@ -103,7 +103,6 @@ | ||||||||||||||||
| }, | |||||||||||||||||
| "dependencies": { | |||||||||||||||||
| "@descope/core-js-sdk": "^2.66.0", | |||||||||||||||||
| "cross-fetch": "^4.0.0", | |||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that cross-fetch should support CF worker - github.com/lquixada/cross-fetch/issues/69 is it not?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Investigated — issue #69 is right for wrangler's own bundler and wrong for everything else. cross-fetch's Worker-safety is a bundler-config coin flip, and it's the flip that lost here.
"main": "dist/node-ponyfill.js", // -> node-fetch -> require('http'/'https')
"browser": "dist/browser-ponyfill.js", // -> safe
"react-native": "dist/react-native-ponyfill.js"There is no
Wrangler puts And the kicker: even on the good path cross-fetch buys us nothing. var ctx = __global__.fetch ? __global__ : __globalThis__;
exports.fetch = ctx.fetchi.e. when a global So: keep it removed. Depending on cross-fetch means depending on every downstream consumer's bundler being configured the way wrangler configures it. |
|||||||||||||||||
| "jose": "5.2.2", | |||||||||||||||||
| "tslib": "^2.0.0" | |||||||||||||||||
| } | |||||||||||||||||
|
|
|||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this removes some specific features we added to customize
highWaterMarkMb, are we ok not supporting it anymore?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch to check, but it was already dead — this PR is deleting a corpse, not a feature.
Three things:
highWaterMarkis a node-fetch@2-only init option. Node's built-in fetch (undici) has no equivalent and silently ignores it. So it only ever did anything on the node-fetch code path.It has been unreachable on every supported runtime since v2.10.0. fix(fetch): prefer native fetch to avoid node-fetch premature close #742 / 5c85f1f (2026-06-18) changed the export to
typeof globalThis.fetch === 'function' ? nativeWrapper : patchedFetch. On Node 18+ that's always the native wrapper, sopatchedFetch— and with it thehighWaterMarkassignment — has only run on Node <18 for the last two releases. Nobody has been getting a 30MB buffer since June.It was never customizable.
grep -rn highWaterMarkacross the repo returns zero hits outside the lines this PR deletes — no config option, no README mention, no test. It was a hardcoded1024 * 1024 * 30constant. The??=meant a caller who happened to stuffhighWaterMarkonto a request init would have it preserved, but that was undocumented and untyped.The bug it originally patched (#149, node-fetch hanging on large responses) is a node-fetch stream bug; undici doesn't have it, and if it did we'd have heard since v2.10.0. Nothing to port forward.