Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
version: [16, 18, 20, 22, 24]
version: [18, 20, 22, 24]
steps:
- name: Checkout code
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
Expand Down Expand Up @@ -45,7 +45,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
version: [16, 18, 20, 22, 24]
version: [18, 20, 22, 24]
steps:
- name: Checkout code
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
Expand Down Expand Up @@ -116,7 +116,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
version: [16, 18, 20, 22, 24]
version: [18, 20, 22, 24]
steps:
- name: Checkout code
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
Expand Down
5 changes: 4 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ for a backend written in Node.js. You can read more on the [Descope Website](htt

## Requirements

The SDK supports Node version 16 and above.
The SDK supports Node version 18 and above.

The SDK uses the runtime's native `fetch` and requires no Node built-in modules, so it also runs
on Cloudflare Workers and other edge runtimes.

## Installing the SDK

Expand Down
21 changes: 21 additions & 0 deletions lib/fetch-polyfill.test.ts
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);
});
});
});
34 changes: 5 additions & 29 deletions lib/fetch-polyfill.ts
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') {

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Contributor

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:

  1. highWaterMark is 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.

  2. 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, so patchedFetch — and with it the highWaterMark assignment — has only run on Node <18 for the last two releases. Nobody has been getting a 30MB buffer since June.

  3. It was never customizable. grep -rn highWaterMark across the repo returns zero hits outside the lines this PR deletes — no config option, no README mention, no test. It was a hardcoded 1024 * 1024 * 30 constant. The ??= meant a caller who happened to stuff highWaterMark onto 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.

// 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;
2 changes: 1 addition & 1 deletion lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ const nodeSdk = ({
}: NodeSdkArgs) => {
const nodeHeaders = {
'x-descope-sdk-name': 'nodejs',
'x-descope-sdk-node-version': process?.versions?.node || '',
'x-descope-sdk-node-version': globalThis.process?.versions?.node || '',
'x-descope-sdk-version': BUILD_VERSION,
};

Expand Down
49 changes: 1 addition & 48 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
"url": "git://github.com/descope/node-sdk.git"
},
"engines": {
"node": ">= 16.0.0"
"node": ">= 18.0.0"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this implies we need to bump major
who uses node16/17?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

},
"scripts": {
"build": "rimraf dist && rollup -c",
Expand Down Expand Up @@ -103,7 +103,6 @@
},
"dependencies": {
"@descope/core-js-sdk": "^2.66.0",
"cross-fetch": "^4.0.0",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

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-ponyfillnode-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.fetch

i.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.

"jose": "5.2.2",
"tslib": "^2.0.0"
}
Expand Down
Loading