Skip to content

experiment: revert Node.js 24 upgrade to test e2e impact #8424

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.

Sign up for GitHub

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

Closed
jacekradko wants to merge 1 commit into from
Closed

experiment: revert Node.js 24 upgrade to test e2e impact #8424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion .github/actions/init-blacksmith/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ inputs:
node-version:
description: 'The node version to use'
required: false
default: '24.15.0'
default: '22'
playwright-enabled:
description: 'Enable Playwright?'
required: false
Expand Down
2 changes: 1 addition & 1 deletion .github/actions/init/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ inputs:
node-version:
description: 'The node version to use'
required: false
default: '24.15.0'
default: '22'
playwright-enabled:
description: 'Enable Playwright?'
required: false
Expand Down
9 changes: 3 additions & 6 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,10 +218,7 @@ jobs:
fail-fast: false
matrix:
include:
- node-version: 24.15.0
test-filter: "**"
filter-label: "**"
- node-version: 20.19.0
- node-version: 22
test-filter: "**"
filter-label: "**"

Expand Down Expand Up @@ -260,7 +257,7 @@ jobs:
- name: Run Typedoc tests
run: |
# Only run Typedoc tests for one matrix version and main test run
if [ "${{ matrix.node-version }}" == "24.15.0" ] && [ "${{ matrix.test-filter }}" = "**" ]; then
if [ "${{ matrix.node-version }}" == "22" ] && [ "${{ matrix.test-filter }}" = "**" ]; then
pnpm turbo run //#test:typedoc
fi
env:
Expand Down Expand Up @@ -504,7 +501,7 @@ jobs:
uses: ./.github/actions/init-blacksmith
with:
turbo-enabled: true
node-version: 24.15.0
node-version: 22
turbo-signature: ${{ secrets.TURBO_REMOTE_CACHE_SIGNATURE_KEY }}
turbo-summarize: ${{ env.TURBO_SUMMARIZE }}
turbo-team: ${{ vars.TURBO_TEAM }}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,7 @@ jobs:

strategy:
matrix:
version: [24] # NOTE: 20 is cached in the main release workflow
version: [22] # NOTE: 18 is cached in the main release workflow

steps:
- name: Checkout Repo
Expand Down
2 changes: 1 addition & 1 deletion .nvmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
24.15.0
22.11.0
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@
},
"packageManager": "pnpm@10.33.0+sha512.10568bb4a6afb58c9eb3630da90cc9516417abebd3fabbe6739f0ae795728da1491e9db5a544c76ad8eb7570f5c4bb3d6c637b2cb41bfdcdb47fa823c8649319",
"engines": {
"node": ">=24.15.0",
"node": ">=22.11.0",
"pnpm": ">=10.33.0"
},
"pnpm": {
Expand Down
8 changes: 2 additions & 6 deletions packages/backend/src/__tests__/proxy.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,11 +572,7 @@ describe('proxy', () => {
expect(response.status).toBe(200);
});

it('omits signal from upstream fetch (Node 24 undici cross-realm AbortSignal)', async () => {
// Node 24's bundled undici tightened the instanceof AbortSignal check on
// RequestInit.signal, which throws on cross-realm signals carried by
// framework Request subclasses. Until we bridge abort propagation via an
// in-realm AbortController, the signal is intentionally omitted.
it('propagates abort signal to upstream fetch', async () => {
const mockResponse = new Response(JSON.stringify({}), { status: 200 });
mockFetch.mockResolvedValue(mockResponse);

Expand All @@ -591,7 +587,7 @@ describe('proxy', () => {
});

const [, options] = mockFetch.mock.calls[0];
expect(options.signal).toBeUndefined();
expect(options.signal).toBe(request.signal);
});

it('includes Cache-Control: no-store on error responses', async () => {
Expand Down
7 changes: 2 additions & 5 deletions packages/backend/src/proxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,15 +297,12 @@ export async function clerkFrontendApiProxy(request: Request, options?: Frontend

try {
// Make the proxied request
// TODO: Restore abort cascade via an in-realm AbortController bridge,
// and consider adding AbortSignal.timeout(30_000) via AbortSignal.any().
// `request.signal` is intentionally omitted: Node 24's bundled undici
// tightened the instanceof AbortSignal check on RequestInit.signal, which
// rejects cross-realm signals carried by framework Request subclasses.
// TODO: Consider adding AbortSignal.timeout(30_000) via AbortSignal.any()
const fetchOptions: RequestInit = {
method: request.method,
headers,
redirect: 'manual',
signal: request.signal,
};

// Only set duplex when body is present (required for streaming bodies)
Expand Down
20 changes: 1 addition & 19 deletions packages/backend/src/tokens/clerkRequest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,25 +26,7 @@ class ClerkRequest extends Request {
// https://github.com/nodejs/undici/issues/2155
// https://github.com/nodejs/undici/blob/7153a1c78d51840bbe16576ce353e481c3934701/lib/fetch/request.js#L854
const url = typeof input !== 'string' && 'url' in input ? input.url : String(input);
// When cloning a Request by passing it as init, hide its `signal`. Undici's
// Request constructor in Node 24 performs a strict instanceof check on the
// signal and rejects ones from a different realm (e.g. NextRequest). Using a
// Proxy keeps property access lazy so environments that don't implement
// optional getters (e.g. Cloudflare Workers' Request lacks `cache`) still work.
let cloneInit: RequestInit | undefined;
if (init) {
cloneInit = init;
} else if (typeof input !== 'string') {
cloneInit = new Proxy(input as Request, {
get(target, prop) {
if (prop === 'signal') {
return undefined;
}
return Reflect.get(target, prop, target);
},
}) as unknown as RequestInit;
}
super(url, cloneInit);
super(url, init || typeof input === 'string' ? undefined : input);
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 30, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's read the file to understand the context
head -40 packages/backend/src/tokens/clerkRequest.ts | cat -n

Repository: clerk/javascript

Length of output: 2270

🏁 Script executed:

# Verify JavaScript operator precedence using Node.js
node <<'EOF'
// Test the precedence claim
// The expression: init || typeof input === 'string' ? undefined : input

// Simulating different values
const test = (init, input) => {
  // Original expression (as reviewer claims it parses)
  const original = (init || typeof input === 'string') ? undefined : input;
  
  // Proposed fix
  const fixed = init || (typeof input === 'string' ? undefined : input);
  
  console.log(`init="${init}", input="${input}"`);
  console.log(`  Original (claimed): ${original}`);
  console.log(`  Fixed: ${fixed}`);
  console.log(`  Different? ${original !== fixed}`);
  console.log('');
};

// Test cases
test(null, 'string');      // init falsy, input is string
test(null, {});            // init falsy, input is object
test({data: 'x'}, 'string'); // init truthy, input is string
test({}, {});              // both truthy
EOF

Repository: clerk/javascript

Length of output: 518

Fix operator precedence bug on Line 29 – init parameter is being dropped.

Due to JavaScript operator precedence, init || typeof input === 'string' ? undefined : input parses as (init || typeof input === 'string') ? undefined : input, causing any provided init to be ignored when it's truthy. This breaks request initialization/cloning when init is passed.

Suggested fix 
-    super(url, init || typeof input === 'string' ? undefined : input);
+    super(url, init || (typeof input === 'string' ? undefined : input));
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/backend/src/tokens/clerkRequest.ts` at line 29, The super call in
the constructor of ClerkRequest uses the expression init || typeof input ===
'string' ? undefined : input which mis-parses due to operator precedence and
drops a truthy init; change the second argument to use init as the primary value
and only fall back to the ternary when init is falsy by parenthesizing the
ternary (i.e., pass init || (typeof input === 'string' ? undefined : input)) so
provided init is preserved; update the super(...) call accordingly.

this.clerkUrl = this.deriveUrlFromHeaders(this);
this.cookies = this.parseCookies(this);
}
Expand Down
4 changes: 1 addition & 3 deletions packages/react-router/src/server/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,12 @@ export const wrapWithClerkState = (data: any) => {
* @internal
*/
export const patchRequest = (request: Request) => {
// Omit `signal` from the clone: Node 24's bundled undici tightened the
// instanceof AbortSignal check, which rejects cross-realm signals (e.g.
// those carried by framework Request subclasses).
const clonedRequest = new Request(request.url, {
headers: request.headers,
method: request.method,
redirect: request.redirect,
cache: request.cache,
signal: request.signal,
});

// If duplex is not set, set it to 'half' to avoid duplex issues with unidici
Expand Down
14 changes: 8 additions & 6 deletions packages/tanstack-react-start/src/__tests__/patchRequest.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,14 @@ describe('patchRequest', () => {
expect(cloned.cache).toBe('no-cache');
});

// The previous "forwards signal aborts" regression test cannot run under Node
// 24 + jsdom + undici: constructing `new Request(url, { signal })` with any
// AbortSignal throws TypeError due to undici's tightened cross-realm
// instanceof check. patchRequest intentionally omits the signal to avoid that
// error; verifying the trade-off in a unit test isn't possible in this
// environment.
it('forwards signal aborts from the original request', () => {
const controller = new AbortController();
const original = new Request('https://example.com/', { signal: controller.signal });
const cloned = patchRequest(original);
expect(cloned.signal.aborted).toBe(false);
controller.abort();
expect(cloned.signal.aborted).toBe(true);
});

it('clones POST requests without forwarding the body', () => {
// patchRequest deliberately omits `body` from the cloned init (see #7020)
Expand Down
7 changes: 1 addition & 6 deletions packages/tanstack-react-start/src/server/utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,17 +69,12 @@ export function getResponseClerkState(requestState: RequestState, additionalStat
* @internal
*/
export const patchRequest = (request: Request) => {
// Omit `signal` from the clone: Node 24's bundled undici tightened the
// instanceof AbortSignal check on RequestInit.signal and rejects any signal
// it does not recognize as its own — including the standard AbortSignal from
// framework Request subclasses or from `new AbortController()`. Until the
// ecosystem stabilizes, abort propagation through this clone is intentionally
// dropped. See packages/backend/src/proxy.ts for the same workaround.
const clonedRequest = new Request(request.url, {
headers: request.headers,
method: request.method,
redirect: request.redirect,
cache: request.cache,
signal: request.signal,
});

// If duplex is not set, set it to 'half' to avoid duplex issues with unidici
Expand Down
Loading