fix(NODE-7603): load os runtime adapter via dynamic import for ESM bundle safety#4979
fix(NODE-7603): load os runtime adapter via dynamic import for ESM bundle safety#4979PavelSafronov wants to merge 7 commits into
Conversation
…ndle safety
resolveRuntimeAdapters no longer calls require('os'), which throws in bundled
ESM output (no `require` in module scope). It resolves the default os adapter
via a dynamic import() constructed with `new Function`, so it survives
TypeScript's module:commonjs downleveling and downstream bundlers. Resolution
is now async: resolveRuntimeAdapters returns Promise<Runtime> with concrete
adapters, and the two consumers (makeClientMetadata, makeKerberosClient) await
it. The CJS-bundled test sandbox (vm) is allowed to use dynamic import so it
keeps working with the new import().
Adds a focused ESM bundling smoke test: bundle resolveRuntimeAdapters to ESM
and run it in Node with no global require.
There was a problem hiding this comment.
Pull request overview
This PR fixes ESM bundling regressions introduced by the runtimeAdapters work by replacing the driver’s require('os') fallback with a dynamic import() approach that survives TypeScript CJS downleveling and downstream bundlers.
Changes:
- Make runtime adapter resolution asynchronous (
resolveRuntimeAdapters(): Promise<Runtime>) and threadPromise<Runtime>through internal options. - Load the default Node
osadapter via a bundler-resistant dynamic import helper. - Update/expand unit tests to validate async runtime resolution and add an esbuild-based ESM bundling regression test; update VM test harness to support dynamic import.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/runtime_adapters.ts |
Switches default os adapter loading to a dynamic import helper; makes adapter resolution async. |
src/mongo_client.ts |
Updates internal MongoOptions.runtime typing to Promise<Runtime>. |
src/cmap/connection.ts |
Updates ConnectionOptions.runtime typing to Promise<Runtime>. |
src/cmap/handshake/client_metadata.ts |
Awaits runtime before reading os for client metadata construction. |
src/cmap/auth/gssapi.ts |
Awaits runtime before using os in Kerberos SPN construction. |
test/unit/runtime_adapters.test.ts |
Adjusts tests to await async runtime resolution and validate adapter behavior. |
test/unit/bundling.test.ts |
Adds regression test that bundles to ESM and verifies no require-scope failure. |
test/tools/utils.ts |
Updates shared test runtime helper to a Promise<Runtime> backed by live Node os. |
test/tools/runner/vm_context_helper.ts |
Configures VM scripts to support dynamic import() used by the driver bundle. |
| } | ||
| function dynamicImport<T>(specifier: string): Promise<T> { | ||
| // eslint-disable-next-line @typescript-eslint/no-implied-eval | ||
| return new Function('specifier', 'return import(specifier)')(specifier); |
There was a problem hiding this comment.
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 --disallow-code-generation-from-strings. We're good with that?
I know it's annoying but trying require('os') first (if require exists and is a function) and then falling back to dynamic import is also an option.
There was a problem hiding this comment.
Nope, definitely not good with breaking bundler package aliasing or --disallow-code-generation-from-strings. Updating to try require('os') first, which should succeed when the environment has a working require (Node, CJS bundling, native ESM). Then if that fails, falling back to eval.
There was a problem hiding this comment.
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
'use strict';
exports.importOs = () => import('node:os');
(and separate .d.ts file alongside to cover TS types)
It's probably my personal preference, but new Function to me looks a bit like eval from old days, and having it as part of production code is something I would prefer to avoid. No strong opinion though.
There was a problem hiding this comment.
@tadjik1 , new Function is definitely eval in a trenchcoat, good instinct.
However, the proposed .js file won't survive our build as-is. Our tsconfig.json has allowJs: true, so a hand-written .js dropped into /src will still be compiled by tsc, downleveling the import() right back to the require() we're trying to avoid. We'd have to modify our build process to drop this .js file into the /lib folder after tsc is done. Doable, if that's what we prefer.
What do you think?
There was a problem hiding this comment.
Yes, that's what I meant: make it pure import('os'), without new Function. How much we would need to modify build process for this?
In general, I would prefer having slightly extended config. @johnmtll @seanrmilligan @nbbeeken what do you think?
There was a problem hiding this comment.
How much we would need to modify build process for this?
Pretty straightforward, actually. We just have to add the shim with import() calls and make sure it doesn't get touched by tsc. Changes added to the PR.
| try { | ||
| const runtime = { | ||
| function loadNodeOsAdapter(): Promise<OsAdapter> { | ||
| if (typeof require === 'function') { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Technically, this can happen in any environment that has a require function, like plain Node, CJS bundling, native ESM. And in some cases (esbuild ESM bundle) the function may be defined, but will always throw, so that's why we have the try/catch around it. Will add this info to the comments.
| ); | ||
| } | ||
|
|
||
| const { os } = await runtime; |
There was a problem hiding this comment.
Do you think it makes sense to rename "runtime" into something like "resolveRuntime" (if possible)?
There was a problem hiding this comment.
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
async connect(): Promise<this> {
if (this.connectionLock) {
return await this.connectionLock;
}
try {
this.connectionLock = this._connect();
await this.connectionLock;
| */ | ||
| function loadNodeOsAdapter(): Promise<OsAdapter> { | ||
| if (typeof require === 'function') { | ||
| // Some environments (plain Node, CJS bundling, native ESM), have a `require` function available, we try that first. |
There was a problem hiding this comment.
we try that first
why can't we just do import?
| Object.defineProperty(exports, '__esModule', { value: true }); | ||
|
|
||
| exports.importOs = function importOs() { | ||
| return import('os'); |
There was a problem hiding this comment.
I think this should be marked as handled 🤔
nbbeeken
left a comment
There was a problem hiding this comment.
Generally speaking I think the approach to pluggable modules would be made a lot easier to maintain through a build tool then via code that handles all the various use cases / platforms / bundling configurations. Emitting the syntax we need here will make it difficult to get wrong, whereas writing javascript and embedding things that build tools normally emit like the __esModule property seems brittle in comparison.
Loosely held opinion! Just offering that esbuild-ing the source will empower a lot more flexibility in the format of the code without having to carefully document exact syntax (like a comment that says "this line must literally appear like this for static import analysis" but isn't actually tested
Description
Summary of Changes
Load
osruntime adapter via dynamic import, so ESM bundle works again.Release Highlight
Bundling the driver into ESM no longer throws
ReferenceError: require is not definedv7.2.0 introduced the experimental
runtimeAdaptersoption and, as part of it, replaced the driver’s static import of Node’sosmodule with a runtimerequire('os'). That works in a CommonJS build, but when the driver is bundled into ESM output (e.g. a Vite/esbuild/rollup server build with"type": "module"), there is norequirein module scope, so constructing a client threwReferenceError: require is not defined. The driver now loads the defaultosadapter through a dynamicimport()that survives bundling, sonew MongoClient()works in ESM bundles. CommonJS usage is unchanged, and supplying your ownruntimeAdapters.oscontinues to work.Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript