-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(NODE-7603): load os runtime adapter via dynamic import for ESM bundle safety #4979
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
e4c1926
1719b83
e6f8d63
dbddcac
ec2729c
31d97a7
3f0e061
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| "files": [ | ||
| "lib", | ||
| "src", | ||
| "shims", | ||
| "etc/prepare.js", | ||
| "mongodb.d.ts", | ||
| "tsconfig.json" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| /** | ||
| * @internal | ||
| * | ||
| * Type declarations for the hand-written CommonJS shim in `runtime_import.js`. That file lives | ||
| * outside `src/` so the TypeScript build never compiles it, which would downlevel its dynamic | ||
| * `import()` to `require()`. | ||
| * | ||
| * @returns A promise that resolves to the `os` module namespace. | ||
| */ | ||
| export declare function importOs(): Promise<typeof import('os')>; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| // This file is hand-written CommonJS. It lives OUTSIDE `src/` on purpose: the TypeScript build | ||
| // only compiles `src/**/*` (see tsconfig.json "include"), so tsc never touches this file. If it | ||
| // did, it would downlevel `import('os')` into `Promise.resolve().then(() => require('os'))` | ||
| // under `module: commonjs`. | ||
| // | ||
| // Keeping the dynamic import in a file the compiler never sees preserves it as a genuine runtime | ||
| // `import()`, which: | ||
| // - survives TypeScript downleveling (the file is not compiled), and | ||
| // - survives downstream bundlers as a real dynamic import, unlike a | ||
| // `new Function('return import(...)')` trick. | ||
| // | ||
| // The specifier is deliberately a literal, not a parameter: a literal `import('os')` remains | ||
| // statically analyzable, so bundlers can see, resolve, and alias the target (NODE-3199), whereas | ||
| // `import(someVariable)` is opaque to static analysis. | ||
| // | ||
| // It ships as-is via the package.json "files" array and is required by the compiled | ||
| // `lib/runtime_adapters.js` as `../shims/runtime_import`. | ||
| // | ||
| // NODE-7133 (ESM-only packages) will eventually let us use `import(...)` directly from TypeScript | ||
| // source and delete this shim. | ||
|
|
||
| Object.defineProperty(exports, '__esModule', { value: true }); | ||
|
|
||
| exports.importOs = function importOs() { | ||
| return import('os'); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,10 +69,7 @@ export class GSSAPI extends AuthProvider { | |
| } | ||
|
|
||
| async function makeKerberosClient({ | ||
| options: { | ||
| hostAddress, | ||
| runtime: { os } | ||
| }, | ||
| options: { hostAddress, runtime }, | ||
| credentials | ||
| }: AuthContext): Promise<KerberosClient> { | ||
| if (!hostAddress || typeof hostAddress.host !== 'string' || !credentials) { | ||
|
|
@@ -81,6 +78,8 @@ async function makeKerberosClient({ | |
| ); | ||
| } | ||
|
|
||
| const { os } = await runtime; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think it makes sense to rename "runtime" into something like "resolveRuntime" (if possible)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We can, but I don't think we should, we already use a similar pattern in other spots, like in https://github.com/mongodb/node-mongodb-native/blob/main/src/mongo_client.ts#L597-L604 |
||
|
|
||
| loadKrb(); | ||
| if ('kModuleError' in krb) { | ||
| throw krb['kModuleError']; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
|
|
||
| import type * as os from 'os'; | ||
|
|
||
| import { importOs } from '../shims/runtime_import'; | ||
| import { type MongoClientOptions } from './mongo_client'; | ||
|
|
||
| /** | ||
|
|
@@ -37,7 +38,7 @@ export interface RuntimeAdapters { | |
| /** | ||
| * @internal | ||
| * | ||
| * Represents a complete, parsed set of runtime adapters. After options parsing, all adapters | ||
| * Represents a complete, parsed set of runtime adapters. After resolution, all adapters | ||
| * are always present (either using the user's provided adapter, or defaulting to the Node.js module). | ||
| */ | ||
| export interface Runtime { | ||
|
|
@@ -48,17 +49,37 @@ export interface Runtime { | |
| * @internal | ||
| * | ||
| * Given a MongoClientOptions, this function resolves the set of runtime options, providing Nodejs implementations if | ||
| * not provided by in `options`, and returns a `Runtime`. | ||
| * not provided in `options`, and returns a `Runtime`. | ||
| * | ||
| * Resolution is asynchronous because the default adapters are loaded from Node.js built-ins via a | ||
| * dynamic `import()` (see `loadNodeOsAdapter`). The resulting promise is created during synchronous | ||
| * options parsing and awaited later by consumers, so the public constructor stays synchronous while | ||
| * the `Runtime` itself exposes fully-resolved, concrete adapters. | ||
| */ | ||
| export function resolveRuntimeAdapters(options: MongoClientOptions): Runtime { | ||
| (globalThis as any)[ALLOWED_DRIVER_REQUIRE_PROPERTY_NAME] = true; | ||
| try { | ||
| const runtime = { | ||
| export async function resolveRuntimeAdapters(options: MongoClientOptions): Promise<Runtime> { | ||
| return { | ||
| os: options.runtimeAdapters?.os ?? (await loadNodeOsAdapter()) | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * @internal | ||
| */ | ||
| function loadNodeOsAdapter(): Promise<OsAdapter> { | ||
| if (typeof require === 'function') { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This pattern signals a CJS build, can we put a comment here explaining that? It's clear with the context of the PR, but may not be clear during future maintenance.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically, this can happen in any environment that has a |
||
| // Some environments (plain Node, CJS bundling, native ESM), have a `require` function available, we try that first. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
why can't we just do |
||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| os: options.runtimeAdapters?.os ?? require('os') | ||
| }; | ||
| return runtime; | ||
| } finally { | ||
| (globalThis as any)[ALLOWED_DRIVER_REQUIRE_PROPERTY_NAME] = false; | ||
| const osModule = require('os') as typeof os; | ||
| return Promise.resolve(osModule); | ||
| } catch { | ||
| // If require fails, we fall back to dynamic import below. | ||
| // This can happen in ESM bundles where `require` may be available, but will always throw. | ||
| } | ||
| } | ||
|
|
||
| // Fall back to a genuine dynamic `import()`. This lives in the hand-written CommonJS shim | ||
| // `../shims/runtime_import`, which is kept out of the TypeScript build so the `import()` is not | ||
| // downleveled to `require()`. | ||
| return importOs(); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| import { spawnSync } from 'node:child_process'; | ||
| import * as fs from 'node:fs'; | ||
| import * as os from 'node:os'; | ||
| import * as path from 'node:path'; | ||
|
|
||
| import { expect } from 'chai'; | ||
| import * as esbuild from 'esbuild'; | ||
| import * as process from 'process'; | ||
| import * as ts from 'typescript'; | ||
|
|
||
| const repoRoot = path.resolve(__dirname, '..', '..'); | ||
|
|
||
| describe('bundling the runtime adapters into ESM output', function () { | ||
| // Transpiling + bundling with esbuild and spawning a fresh node process can exceed the default timeout. | ||
| this.timeout(120_000); | ||
|
|
||
| let tmpDir: string; | ||
| let esmBundlePath: string; | ||
|
|
||
| before('compile and bundle resolveRuntimeAdapters into ESM', async function () { | ||
| // NODE-7603: resolveRuntimeAdapters used to call require('os'), which throws in bundled ESM | ||
| // output (no `require` in module scope). This reproduces the published artifact and a downstream | ||
| // ESM bundler: transpile the source the way the build does (module: commonjs) so any literal | ||
| // `import()` would be downleveled back to require(), then bundle that to ESM. The fix must | ||
| // therefore survive both steps and resolve `os` via a real runtime import(). | ||
| const source = fs.readFileSync(path.join(repoRoot, 'src', 'runtime_adapters.ts'), 'utf8'); | ||
| const { outputText: compiledCjs } = ts.transpileModule(source, { | ||
| compilerOptions: { module: ts.ModuleKind.CommonJS, target: ts.ScriptTarget.ES2023 } | ||
| }); | ||
|
|
||
| // Mirror the published package layout: the compiled module lives in lib/ and requires the | ||
| // hand-written shim as `../shims/runtime_import`, so reproduce those sibling directories. | ||
| tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'mongodb-esm-bundle-')); | ||
| fs.mkdirSync(path.join(tmpDir, 'lib')); | ||
| fs.mkdirSync(path.join(tmpDir, 'shims')); | ||
| fs.writeFileSync(path.join(tmpDir, 'lib', 'runtime_adapters.js'), compiledCjs); | ||
| // runtime_import.js is the hand-written CommonJS shim that resolveRuntimeAdapters imports for | ||
| // its dynamic import() fallback. It is deliberately NOT compiled (that is the whole point of | ||
| // NODE-7603), so copy it verbatim into the sibling shims/ dir, exactly as it ships. | ||
| fs.copyFileSync( | ||
| path.join(repoRoot, 'shims', 'runtime_import.js'), | ||
| path.join(tmpDir, 'shims', 'runtime_import.js') | ||
| ); | ||
| esmBundlePath = path.join(tmpDir, 'app.mjs'); | ||
|
|
||
| await esbuild.build({ | ||
| stdin: { | ||
| // No user-provided os adapter, so resolveRuntimeAdapters falls back to loading Node's os. | ||
| contents: ` | ||
| import { resolveRuntimeAdapters } from './lib/runtime_adapters.js'; | ||
| const runtime = await resolveRuntimeAdapters({}); | ||
| const osAdapter = await runtime.os; | ||
| if (typeof osAdapter.platform !== 'function') { | ||
| throw new Error('resolved os adapter is missing platform()'); | ||
| } | ||
| console.log('resolved os adapter'); | ||
| `, | ||
| resolveDir: tmpDir, | ||
| loader: 'js' | ||
| }, | ||
| bundle: true, | ||
| outfile: esmBundlePath, | ||
| platform: 'node', | ||
| format: 'esm', | ||
| target: 'node20', | ||
| logLevel: 'silent' | ||
| }); | ||
| }); | ||
|
|
||
| after(function () { | ||
| if (tmpDir) fs.rmSync(tmpDir, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| it('resolves the default os adapter without a global require', function () { | ||
| const { status, stdout, stderr } = spawnSync(process.execPath, [esmBundlePath], { | ||
| encoding: 'utf8' | ||
| }); | ||
|
|
||
| // Surface the child's stderr in the failure message so a regression is easy to diagnose. | ||
| expect(stderr, stderr).to.not.match(/require/i); | ||
| expect(stdout).to.include('resolved os adapter'); | ||
| expect(status).to.equal(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be marked as handled 🤔