Skip to content

Commit d194f1d

Browse files
Use shim pattern for CORS detection instead of globalThis.document check
Per review feedback, replace the runtime feature-sniff (globalThis.document) with the package's existing _shims export-conditions pattern. Adds CORS_IS_POSSIBLE constant to the client shims: - shimsNode.ts -> false (Node has no CORS) - shimsWorkerd.ts -> false (Cloudflare Workers has no CORS) - shimsBrowser.ts -> true (new file; browser condition now resolves here) This also fixes the Web Worker / Service Worker gap noted in the PR description: bundlers resolve those to the 'browser' condition, so CORS_IS_POSSIBLE is correctly true there — unlike the document check which would have returned false in workers. Tests now mock the shim module rather than stubbing globalThis.document.
1 parent 5af5343 commit d194f1d

File tree

7 files changed

+49
-17
lines changed

7 files changed

+49
-17
lines changed

packages/client/package.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,8 @@
3030
"import": "./dist/shimsWorkerd.mjs"
3131
},
3232
"browser": {
33-
"types": "./dist/shimsWorkerd.d.mts",
34-
"import": "./dist/shimsWorkerd.mjs"
33+
"types": "./dist/shimsBrowser.d.mts",
34+
"import": "./dist/shimsBrowser.mjs"
3535
},
3636
"node": {
3737
"types": "./dist/shimsNode.d.mts",

packages/client/src/client/auth.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { CORS_IS_POSSIBLE } from '@modelcontextprotocol/client/_shims';
12
import type {
23
AuthorizationServerMetadata,
34
FetchLike,
@@ -844,13 +845,10 @@ export async function discoverOAuthProtectedResourceMetadata(
844845
* error object alone, so the swallow-and-fallthrough heuristic is preserved there.
845846
*/
846847
async function fetchWithCorsRetry(url: URL, headers?: Record<string, string>, fetchFn: FetchLike = fetch): Promise<Response | undefined> {
847-
// CORS only exists in browsers. In Node.js/Workers/etc., a TypeError from fetch is always a
848-
// real network or configuration error, never CORS.
849-
const corsIsPossible = (globalThis as { document?: unknown }).document !== undefined;
850848
try {
851849
return await fetchFn(url, { headers });
852850
} catch (error) {
853-
if (!(error instanceof TypeError) || !corsIsPossible) {
851+
if (!(error instanceof TypeError) || !CORS_IS_POSSIBLE) {
854852
throw error;
855853
}
856854
if (headers) {
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
/**
2+
* Browser runtime shims for client package
3+
*
4+
* This file is selected via package.json export conditions when running in a browser.
5+
*/
6+
export { CfWorkerJsonSchemaValidator as DefaultJsonSchemaValidator } from '@modelcontextprotocol/core';
7+
8+
/**
9+
* Whether `fetch()` may throw `TypeError` due to CORS. Only true in browser contexts
10+
* (including Web Workers / Service Workers). In Node.js and Cloudflare Workers, a
11+
* `TypeError` from `fetch` is always a real network/configuration error.
12+
*/
13+
export const CORS_IS_POSSIBLE = true;

packages/client/src/shimsNode.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,3 +4,10 @@
44
* This file is selected via package.json export conditions when running in Node.js.
55
*/
66
export { AjvJsonSchemaValidator as DefaultJsonSchemaValidator } from '@modelcontextprotocol/core';
7+
8+
/**
9+
* Whether `fetch()` may throw `TypeError` due to CORS. CORS is a browser-only concept —
10+
* in Node.js, a `TypeError` from `fetch` is always a real network/configuration error
11+
* (DNS resolution, connection refused, invalid URL), never a CORS error.
12+
*/
13+
export const CORS_IS_POSSIBLE = false;

packages/client/src/shimsWorkerd.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,3 +4,10 @@
44
* This file is selected via package.json export conditions when running in workerd.
55
*/
66
export { CfWorkerJsonSchemaValidator as DefaultJsonSchemaValidator } from '@modelcontextprotocol/core';
7+
8+
/**
9+
* Whether `fetch()` may throw `TypeError` due to CORS. CORS is a browser-only concept —
10+
* in Cloudflare Workers, a `TypeError` from `fetch` is always a real network/configuration
11+
* error, never a CORS error.
12+
*/
13+
export const CORS_IS_POSSIBLE = false;

packages/client/test/client/auth.test.ts

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -34,25 +34,32 @@ const mockFetch = vi.fn();
3434
globalThis.fetch = mockFetch;
3535

3636
/**
37-
* fetchWithCorsRetry gates its CORS-swallowing heuristic on running in a browser (where `document`
38-
* exists). CORS doesn't apply in Node.js, so there a fetch TypeError is always a real network error
39-
* and is thrown instead of swallowed. Tests that specifically exercise the CORS retry logic call
40-
* this helper to simulate a browser environment. Cleanup is done by `restoreBrowserLikeEnvironment`
41-
* in an `afterEach` hook so a failed assertion can't leak the `document` stub into later tests.
37+
* fetchWithCorsRetry gates its CORS-swallowing heuristic on the `CORS_IS_POSSIBLE` shim constant.
38+
* Tests run under the Node shim (`false`), so a fetch TypeError is treated as a real network error
39+
* and thrown instead of swallowed. Tests that specifically exercise the browser CORS retry path
40+
* call `withBrowserLikeEnvironment()` to flip the mocked constant to `true`. The `afterEach` hook
41+
* resets it so a failed assertion can't leak the override into later tests.
4242
*/
43+
let mockedCorsIsPossible = false;
44+
vi.mock('@modelcontextprotocol/client/_shims', async importOriginal => {
45+
const actual = await importOriginal<typeof import('@modelcontextprotocol/client/_shims')>();
46+
return {
47+
...actual,
48+
get CORS_IS_POSSIBLE() {
49+
return mockedCorsIsPossible;
50+
}
51+
};
52+
});
4353
function withBrowserLikeEnvironment(): void {
44-
(globalThis as { document?: unknown }).document = {};
45-
}
46-
function restoreBrowserLikeEnvironment(): void {
47-
delete (globalThis as { document?: unknown }).document;
54+
mockedCorsIsPossible = true;
4855
}
4956

5057
describe('OAuth Authorization', () => {
5158
beforeEach(() => {
5259
mockFetch.mockReset();
5360
});
5461
afterEach(() => {
55-
restoreBrowserLikeEnvironment();
62+
mockedCorsIsPossible = false;
5663
});
5764

5865
describe('extractWWWAuthenticateParams', () => {

packages/client/tsdown.config.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { defineConfig } from 'tsdown';
33
export default defineConfig({
44
// 1. Entry Points
55
// Directly matches package.json include/exclude globs
6-
entry: ['src/index.ts', 'src/shimsNode.ts', 'src/shimsWorkerd.ts'],
6+
entry: ['src/index.ts', 'src/shimsNode.ts', 'src/shimsWorkerd.ts', 'src/shimsBrowser.ts'],
77

88
// 2. Output Configuration
99
format: ['esm'],

0 commit comments

Comments
 (0)