feat(fetch): use native global fetch instead of cross-fetch for workers - #777
feat(fetch): use native global fetch instead of cross-fetch for workers#777omercnet wants to merge 3 commits into
Conversation
cross-fetch resolves to node-fetch, which requires node's http/https at import time. In Cloudflare Workers (wrangler nodejs_compat / Nitro) unenv stubs those modules with functions that throw, so any request through the SDK failed with "[unenv] https.request is not implemented yet!". Native fetch is available on every runtime the SDK now supports, so the polyfill collapses to a thin indirection over globalThis.fetch and no bundler can pull node built-ins into an edge build. Dropped along with it: the globalThis.Headers assignment and the 30MB highWaterMark patch, both node-fetch-only workarounds. Also reads process via globalThis, since a bare `process` is a ReferenceError in a Worker without nodejs_compat. Requires node 18+ (node 16 has been EOL since Sept 2023).
|
🐕 Uh oh! I ran into an error. Please check the logs for details. |
🐕 Suggested ReviewersThe review assignment covers primary areas impacted by the change: fetch polyfill logic and main index file modifications. Selected reviewers have recent contributions in these core files, ensuring relevant expertise and broad coverage of the affected code.
Suggested by Shuni based on git history and PR context. Names are not @-mentioned to avoid notifying anyone — request a review from whoever fits best. |
![shuni-bot[bot]](https://avatars.githubusercontent.com/in/2536750?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐕 Shuni's Review
Drops cross-fetch and collapses fetch-polyfill.ts to a thin wrapper over native fetch, bumps engines to Node 18+, and fixes the bare process reference that ReferenceErrors on Workers.
Verified: the removed globalThis.Headers ??= Headers is safe (Node ≥18 exposes Headers/fetch as writable globals, and jest-environment-node copies them, so index.test.ts:669 and the new jest.spyOn(globalThis, 'fetch') both hold). Hand-edited lockfile is consistent — no residual cross-fetch/node-fetch/whatwg-url/tr46/webidl-conversions refs, lockfileVersion 3 so no legacy section to sync.
Sniffed out 1 issue:
- 1 🟡 MEDIUM: Node 16 drop ships as a minor with no fetch fallback
Heads-up: CI will be red here for an unrelated pre-existing reason — HEAD's lockfile pins typescript@4.7.4 while package.json wants ^7.0.0, so npm ci fails in all four jobs. Good bones otherwise. Woof!
| }, | ||
| "engines": { | ||
| "node": ">= 16.0.0" | ||
| "node": ">= 18.0.0" |
There was a problem hiding this comment.
🟡 MEDIUM: This is a breaking change for Node 16 consumers, but the PR is titled feat: so release-please will cut a minor (2.14.0).
engines is only a warning by default (not engine-strict), so a Node 16 user on ^2.x installs 2.14.0 successfully and then every SDK request throws TypeError: globalThis.fetch is not a function — the cross-fetch fallback that used to cover them is gone, and there's no guard or actionable error.
Node 16 is EOL so dropping it is reasonable, but consider a major (or at minimum an explicit release-note callout).
There was a problem hiding this comment.
Valid, and @asafshen agreed — this is now a major. 11b8ad5 adds a BREAKING CHANGE: footer; the PR title also needs feat(fetch)!: since the repo squash-merges (details in the thread above).
That closes the escape hatch you identified: on a major, a Node 16 consumer on ^2.x never receives this release at all. Reaching the TypeError now requires explicitly installing @latest on an EOL runtime past an EBADENGINE warning, so I did not add a runtime guard — it'd be code for a runtime we're deliberately dropping. Cheap to add later (three lines in fetch-polyfill.ts) if a real ticket shows up.
|
🐕 Shuni hit a problem and couldn't finish. |
| }, | ||
| "engines": { | ||
| "node": ">= 16.0.0" | ||
| "node": ">= 18.0.0" |
There was a problem hiding this comment.
this implies we need to bump major
who uses node16/17?
There was a problem hiding this comment.
Agreed — going major. Pushed 11b8ad5, a marker commit with a BREAKING CHANGE: footer.
One thing you need to do, though: this repo squash-merges (every commit on main ends in (#NNN)), so release-please reads the PR title, not the commit bodies. The title needs the !:
feat(fetch)!: use native global fetch instead of cross-fetch for workers
The pr-title-check job accepts ! fine. The marker commit is only a fallback for the case where the squash body keeps the default commit list — the title is the authoritative lever, and I can't set it from here.
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, ^2.x consumers never auto-upgrade, so the failure mode @shuni-bot flagged in the sibling thread needs someone to explicitly npm i @descope/node-sdk@latest on Node 16 and click past an EBADENGINE warning — which is what engines + a major bump is for. I skipped adding a runtime guard for that; worth adding only if support actually sees such a ticket.
| }, | ||
| "dependencies": { | ||
| "@descope/core-js-sdk": "^2.66.0", | ||
| "cross-fetch": "^4.0.0", |
There was a problem hiding this comment.
I see that cross-fetch should support CF worker - github.com/lquixada/cross-fetch/issues/69
is it not?
There was a problem hiding this comment.
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.
cross-fetch@4.1.0's package.json:
"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 exports map. That's the crux: without exports, no worker/workerd export condition can be honored, so the only thing that steers a bundler away from node-ponyfill is the legacy browser mainField. I ran the resolution matrix through esbuild against a real cross-fetch@4.1.0 install:
| bundler config | resolves to | pulls http/https |
|---|---|---|
platform: node (mainFields ['main','module']) |
node-ponyfill → node-fetch |
✅ yes |
platform: browser |
browser-ponyfill |
no |
platform: neutral + conditions: [workerd, worker, browser] |
ERROR: Could not resolve "cross-fetch" |
— |
mainFields: ['browser','module','main'] |
browser-ponyfill |
no |
Wrangler puts browser first in mainFields, so plain wrangler dev/deploy gets the good path — that's what #69 is reporting. But a toolchain that resolves with node mainFields and enables nodejs_compat (Nitro/Nuxt cloudflare preset, next-on-pages, and anything else driving unenv) gets node-ponyfill, and unenv then stubs https with a throwing mock. That is literally the reported error: [unenv] https.request is not implemented yet!. Note row 3 too — being explicit about worker conditions doesn't rescue it, it makes it worse.
And the kicker: even on the good path cross-fetch buys us nothing. browser-ponyfill.js ends with
var ctx = __global__.fetch ? __global__ : __globalThis__;
exports.fetch = ctx.fetchi.e. when a global fetch exists (always, on any Worker) it hands back the native one — the same thing our 5-line file now does directly, minus ~600 lines of dead XHR-based whatwg-fetch shim (and XMLHttpRequest doesn't exist in Workers anyway, so that fallback could never have worked there). It also hands it back unbound, which is the illegal-invocation footgun our wrapper exists to avoid.
So: keep it removed. Depending on cross-fetch means depending on every downstream consumer's bundler being configured the way wrangler configures it.
| // 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') { |
There was a problem hiding this comment.
this removes some specific features we added to customize highWaterMarkMb , are we ok not supporting it anymore?
There was a problem hiding this comment.
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.
|
@shuni see review and investigate |
|
🐕 Done! Updated PR: #777 📋 Actions TakenView AI responseInvestigated all four threads and replied to each. One commit, Thread 3 — "cross-fetch should support CF worker, is it not?" (the interesting one)
Wrangler puts And on the good path cross-fetch just hands back Thread 4 — highWaterMarkMb. Already dead code, not a feature being dropped: it's a node-fetch@2-only init option (undici ignores it), it's been unreachable on Node 18+ since #742/ Threads 1 + 2 — major bump. Agreed, added Also: the PR body's caveat about |
BREAKING CHANGE: the SDK now requires Node 18+ and uses the runtime native fetch. Node 16 and 17 (EOL since Sept 2023 and June 2022) no longer get the bundled cross-fetch/node-fetch fallback. Marker commit so release-please cuts a major. Under squash-merge the PR title is authoritative, so it also needs the `!`: `feat(fetch)!: ...`.
2 participants
Fixes descope/etc#14233
View Shuni run
Done — committed as
2170384.What changed
lib/fetch-polyfill.ts— collapsed to a one-line indirection overglobalThis.fetch; removed thecross-fetchimport, theglobalThis.Headersassignment, and the 30MBhighWaterMarkpatch (node-fetch-only). Module path kept, so theauthz/fgatest mocks are untouched.lib/index.ts:55—globalThis.process?.versions?.node(bareprocessis aReferenceErrorin a Worker).package.json— droppedcross-fetch,engines.node→>= 18.0.0; lockfile edited surgically (48 lines:cross-fetch,node-fetch,whatwg-url,tr46,webidl-conversions— nothing else referenced them)..github/workflows/ci.yml— dropped16from the three node matrices; the new test spies onglobalThis.fetch, which doesn't exist there.lib/fetch-polyfill.test.ts— delegation test + a dist assertion (cross-fetch/node-fetch/node:http/node:httpsabsent fromdist/index.esm.js).README.md— Node 18+, runs on Workers/edge, no Node built-ins.Two things to know
npm installandnpm run buildare broken onmain, independently of this: commit b7b6e0e bumpedtypescriptto^7but leftpackage-lock.jsonat^4.6.4(sonpm ciis out of sync), androllup-plugin-dts@4peer-requirestypescript@^4.1.@rollup/plugin-typescriptalso crashes on TS 7. I built and tested withtypescript@5.9.3installed locally with--no-save --no-package-lock— nothing committed. That needs its own fix.I did not run the
wrangler devrepro (§5.1) — no Workers runtime in this sandbox. It's worth running against this build to close the loop, but the failure mode is removed either way: there is no longer any path tonode-fetch, regardless of whether the customer was pre-2.8.0 or hit the global-timing case.Not committed:
feat:was chosen overfix:so release-please cuts a minor, per §6's recommendation for theenginesbump — switch tofix:if a patch is preferred.Created by Shuni 🐕