Skip to content

fix(server-runtime-injection): Keep ES modules working on Deno #24669

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

Merged
JPeer264 merged 1 commit into from
Sep 27, 2026
Merged

fix(server-runtime-injection): Keep ES modules working on Deno #24669

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
71 changes: 57 additions & 14 deletions packages/server-runtime-injection/src/register.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { consoleSandbox, debug, getClient, GLOBAL_OBJ, parseSemver } from '@sentry/core';
import { existsSync } from 'node:fs';
import { existsSync, readFileSync } from 'node:fs';
import * as Module from 'node:module';
import { dirname, join } from 'node:path';
import { fileURLToPath, pathToFileURL } from 'node:url';
Expand Down Expand Up @@ -28,20 +28,63 @@ function hasStableSyncModuleHooks(isDeno: boolean): boolean {
return major > 25 || (major === 25 && minor >= 1) || (major === 24 && minor >= 13);
}

/** `"type"` of the nearest `package.json`, keyed by the directory the lookup started in. */
const packageTypeByDir = new Map<string, string | undefined>();

function getPackageType(dir: string): string | undefined {
if (packageTypeByDir.has(dir)) {
return packageTypeByDir.get(dir);
}

let type: string | undefined;
const packageJsonPath = join(dir, 'package.json');
if (existsSync(packageJsonPath)) {
try {
type = (JSON.parse(readFileSync(packageJsonPath, 'utf8')) as { type?: string }).type;
} catch {
type = undefined;
}
} else if (dirname(dir) !== dir) {
type = getPackageType(dirname(dir));
}

packageTypeByDir.set(dir, type);
return type;
}

/** The `format` Node would report for `url`, for the formats Deno leaves out. */
function getMissingDenoFormat(url: string): string | undefined {
if (url.endsWith('.json')) {
return 'json';
}
if (url.endsWith('.mjs')) {
return 'module';
}
if (url.startsWith('file:') && url.endsWith('.js') && getPackageType(dirname(fileURLToPath(url))) === 'module') {
return 'module';
}
return undefined;
}

/**
* Deno's `nextLoad` reports no `format` for a `.json` file, where Node reports `'json'`. With any
* load hook installed, Deno's CJS loader then compiles the JSON as JavaScript and `require()` of it
* throws `SyntaxError: Unexpected token ':'`. Restoring the format is enough, and only Deno needs
* it: on Node the format is never missing.
* Deno's `nextLoad` reports no `format` for a `.json` file or an ES module, where Node reports
* `'json'` or `'module'`. Without the format, Deno's CJS loader compiles JSON as JavaScript
* (`SyntaxError: Unexpected token ':'`), and the transform treats an ES module as CommonJS and
Comment on lines +63 to +72

@sentry sentry Bot Sep 27, 2026

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.

Bug: The getPackageType function incorrectly caches undefined and fails to check parent directories if reading an existing package.json file fails.
Severity: LOW

Suggested Fix

When readFileSync fails within the try...catch block, the function should not cache undefined. Instead, it should fall back to checking the parent directory, similar to the logic used when existsSync returns false.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: packages/server-runtime-injection/src/register.ts#L63-L72

Potential issue: The function `getPackageType` can incorrectly cache `undefined` for a
directory. This occurs if a `package.json` file exists but is unreadable (e.g., due to a
race condition or permissions issue). When `readFileSync` fails, the `catch` block sets
the module `type` to `undefined`. This `undefined` value is then cached for the
directory. The logic does not proceed to check parent directories for a `package.json`
file in this failure case, which can lead to incorrect module type resolution for
subsequent lookups in that directory.

Did we get this right? 👍 / 👎 to inform future reviews.

* injects a `require()` into it (`ReferenceError: require is not defined`). The format is restored
* on the `nextLoad` result, so the transform sees it too. Only Deno needs this.

@isaacs isaacs Sep 25, 2026

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.

Worth adding a comment here and in the PR description as well, that it's only needed as long as we support Deno versions that do not have the fix in denoland/deno#36849.

We're already gating on the presence/lack of a format, so we don't need any version sniffing, I don't think. But it'd be nice to know when we can cut the fix out entirely, certainly not before v12.

Actually, come to think of it, could also add a // todo(v12): evaluate if this is still needed for supported Deno versions so we know to circle back.

*/
function withDenoJsonFormat(loadHook: Function): Function {
return (url: string, context: unknown, nextLoad: Function) => {
const result = loadHook(url, context, nextLoad) as { format?: string };
if (result?.format === undefined && url.endsWith('.json')) {
result.format = 'json';
}
return result;
};
function withDenoFormats(loadHook: Function): Function {
return (url: string, context: unknown, nextLoad: Function) =>
loadHook(url, context, (nextUrl: string, nextContext: unknown) => {
const result = nextLoad(nextUrl, nextContext) as { format?: string | null } | undefined;
if (result && result.format == null) {
const format = getMissingDenoFormat(nextUrl);
if (format) {
result.format = format;
}
}
return result;
});
}

/**
Expand Down Expand Up @@ -181,7 +224,7 @@ export function registerDiagnosticsChannelInjection(): void {
try {
if (typeof mod.registerHooks === 'function' && stableSyncHooks) {
initialize({ instrumentations: SENTRY_RUNTIME_INSTRUMENTATIONS });
mod.registerHooks({ resolve, load: globalAny.Deno ? withDenoJsonFormat(load) : load });
mod.registerHooks({ resolve, load: globalAny.Deno ? withDenoFormats(load) : load });
debug.log('Registered diagnostics-channel injection via Module.registerHooks()');
} else if (typeof mod.register === 'function' && !globalAny.Bun && !globalAny.Deno) {
// `Module.register` + the `_compile` patch is Node 18.19–24.12 / 25.0
Expand Down
85 changes: 82 additions & 3 deletions packages/server-runtime-injection/test/register.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,27 @@
import type * as SentryCore from '@sentry/core';
import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs';
import type * as NodeModule from 'node:module';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import { tmpdir } from 'node:os';
import { join } from 'node:path';
import { pathToFileURL } from 'node:url';
import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest';

type LoadResult = { format?: string | null };
type LoadHook = (url: string, context: unknown, nextLoad: (url: string, context: unknown) => LoadResult) => unknown;

// The registration installs real Node module hooks, which we neither want nor need here. Stub the
// tracing-hooks surface so the tests can drive the diagnostics callback directly, and neuter
// `node:module`'s hook installers: on Node 24.13+/26 the stable-sync-hooks path would otherwise call
// the real `Module.registerHooks({ resolve, load })` with the mocked (undefined-returning) callbacks,
// leaving a broken resolve hook installed process-wide that crashes vitest's next dynamic `import()`.
const registerHooksMock = vi.fn<(options: { load: LoadHook; resolve: unknown }) => void>();
vi.mock('node:module', async importOriginal => {
const actual = await importOriginal<typeof NodeModule>();
return { ...actual, registerHooks: vi.fn(), register: vi.fn() };
return {
...actual,
registerHooks: (options: { load: LoadHook; resolve: unknown }) => registerHooksMock(options),
register: vi.fn(),
};
});

const setDiagnosticsHookMock = vi.fn<(cb: DiagnosticsCallback) => void>();
Expand All @@ -21,9 +33,10 @@ vi.mock('@apm-js-collab/tracing-hooks', () => ({
patch(): void {}
},
}));
const loadMock = vi.fn<LoadHook>();
vi.mock('@apm-js-collab/tracing-hooks/hook-sync.mjs', () => ({
initialize: vi.fn(),
load: vi.fn(),
load: (...args: Parameters<LoadHook>) => loadMock(...args),
resolve: vi.fn(),
createDiagnosticsPort: vi.fn(),
}));
Expand Down Expand Up @@ -192,3 +205,69 @@ describe('registerDiagnosticsChannelInjection - bundled/tree-shaken detection',
expect(setDiagnosticsHookMock).toHaveBeenCalledTimes(1);
});
});

describe('registerDiagnosticsChannelInjection - Deno module formats', () => {
let fixtureDir: string;
let registerDiagnosticsChannelInjection: typeof RegisterModule.registerDiagnosticsChannelInjection;
let loadHook: LoadHook;

beforeAll(() => {
fixtureDir = mkdtempSync(join(tmpdir(), 'sentry-deno-formats-'));
mkdirSync(join(fixtureDir, 'esm-package', 'lib'), { recursive: true });
writeFileSync(join(fixtureDir, 'esm-package', 'package.json'), JSON.stringify({ type: 'module' }));
writeFileSync(join(fixtureDir, 'esm-package', 'lib', 'index.js'), 'export default 1;');
mkdirSync(join(fixtureDir, 'cjs-package'), { recursive: true });
writeFileSync(join(fixtureDir, 'cjs-package', 'package.json'), JSON.stringify({}));
writeFileSync(join(fixtureDir, 'cjs-package', 'index.js'), 'module.exports = 1;');
});

afterAll(() => {
rmSync(fixtureDir, { recursive: true, force: true });
});

beforeEach(async () => {
delete GLOBAL_OBJ.__SENTRY_ORCHESTRION__;
(globalThis as { Deno?: unknown }).Deno = { version: { deno: '2.8.3' } };
vi.resetModules();
registerHooksMock.mockClear();
// The transform reads the format from what `nextLoad` returns, so the stub forwards to it.
loadMock.mockImplementation((url, context, nextLoad) => nextLoad(url, context));

({ registerDiagnosticsChannelInjection } = await import('../src/register'));
registerDiagnosticsChannelInjection();

const [options] = registerHooksMock.mock.lastCall ?? [];
if (!options) {
throw new Error('registerDiagnosticsChannelInjection() did not call Module.registerHooks()');
}
loadHook = options.load;
});

afterEach(() => {
delete GLOBAL_OBJ.__SENTRY_ORCHESTRION__;
delete (globalThis as { Deno?: unknown }).Deno;
loadMock.mockReset();
});

it.each([
['a `.js` file in a `"type": "module"` package', 'esm-package/lib/index.js', 'module'],
['an `.mjs` file', 'cjs-package/other.mjs', 'module'],
['a `.json` file', 'cjs-package/package.json', 'json'],
])('restores the format Deno leaves out for %s', (_label, file, format) => {
const url = pathToFileURL(join(fixtureDir, file)).href;

expect(loadHook(url, {}, () => ({ format: null }))).toEqual({ format });
});

it('keeps the format missing for a `.js` file in a package without `"type": "module"`', () => {
const url = pathToFileURL(join(fixtureDir, 'cjs-package', 'index.js')).href;

expect(loadHook(url, {}, () => ({ format: null }))).toEqual({ format: null });
});

it('keeps a format that Deno reports', () => {
const url = pathToFileURL(join(fixtureDir, 'esm-package', 'lib', 'index.js')).href;

expect(loadHook(url, {}, () => ({ format: 'commonjs' }))).toEqual({ format: 'commonjs' });
});
});
Loading