-
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 3 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 |
|---|---|---|
|
|
@@ -37,7 +37,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 +48,56 @@ 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 async function resolveRuntimeAdapters(options: MongoClientOptions): Promise<Runtime> { | ||
| return { | ||
| os: options.runtimeAdapters?.os ?? (await loadNodeOsAdapter()) | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * @internal | ||
| */ | ||
| export function resolveRuntimeAdapters(options: MongoClientOptions): Runtime { | ||
| (globalThis as any)[ALLOWED_DRIVER_REQUIRE_PROPERTY_NAME] = true; | ||
| try { | ||
| const runtime = { | ||
| 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 |
||
| 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. | ||
| } | ||
| } | ||
| return dynamicImport<typeof os>('os'); | ||
| } | ||
|
|
||
| /** | ||
| * @internal | ||
| * | ||
| * Dynamically imports a module at runtime in a way that survives bundling and TypeScript's | ||
| * downleveling. We deliberately avoid both `require(specifier)` and a static `await import(...)`: | ||
| * - a raw `require` throws in bundled ESM output, where there is no `require` in module scope | ||
| * (NODE-7603), and | ||
| * - a literal `import(...)` is downleveled back to `require(...)` by TypeScript under | ||
| * `module: commonjs`, which reintroduces the same problem in the published CommonJS build. | ||
| * | ||
| * Constructing the dynamic `import` through `new Function` hides it from both the TypeScript | ||
| * compiler and downstream bundlers, so it survives as a real runtime `import()` in every | ||
| * environment. Call this lazily (never at module load) so strict-CSP runtimes that forbid | ||
| * `new Function` are unaffected unless they actually need the default adapter. | ||
| * | ||
| * NODE-7133 (ESM-only packages) will eventually let us use `import(...)` directly and drop this. | ||
| * | ||
| * @param specifier - The module specifier to import. | ||
| * @returns A promise that resolves to the imported module. | ||
| */ | ||
| function dynamicImport<T>(specifier: string): Promise<T> { | ||
| // eslint-disable-next-line @typescript-eslint/no-implied-eval | ||
| return new Function('specifier', 'return import(specifier)')(specifier); | ||
|
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 breaks things like bundler package aliasing (which I know the whole runtime adapter options work is intended to make unnecessary) and Node.js's I know it's annoying but trying
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. Nope, definitely not good with breaking bundler package aliasing or
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. What do you think about handwriting a bit of .js to make it "survives bundling and TypeScript's" - basically take this code out of TS compiler? As simple as (and separate .d.ts file alongside to cover TS types) It's probably my personal preference, but
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. @tadjik1 , However, the proposed What do you think?
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. Yes, that's what I meant: make it pure
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.
Pretty straightforward, actually. We just have to add the shim with |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| 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 } | ||
| }); | ||
|
|
||
| tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'mongodb-esm-bundle-')); | ||
| fs.writeFileSync(path.join(tmpDir, 'runtime_adapters.js'), compiledCjs); | ||
| 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 './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.
Do you think it makes sense to rename "runtime" into something like "resolveRuntime" (if possible)?
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.
runtimePromise?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