Skip to content

fix: Suppress node 23 warnings about require-ing ESM MONGOSH-1895 #488

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

Closed
wants to merge 1 commit into from
Closed

fix: Suppress node 23 warnings about require-ing ESM MONGOSH-1895 #488

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
32 changes: 31 additions & 1 deletion packages/devtools-proxy-support/src/fetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@ async function importNodeFetch(): Promise<typeof fetch> {
// Node-fetch is an ESM module from 3.x
// Importing ESM modules to CommonJS is possible with a dynamic import.
// However, once this is transpiled with TS, `await import()` changes to `require()`, which fails to load
// the package at runtime.
// the package at runtime for Node < 23.
// The alternative, to transpile with "moduleResolution": "NodeNext", is not always feasible.
// Use this function to safely import the node-fetch package
let module: typeof fetch | { default: typeof fetch };
try {
suppressExperimentalWarningsIfNecessary();
module = await import('node-fetch');
} catch (err: unknown) {
if (
Expand All @@ -37,6 +38,35 @@ async function importNodeFetch(): Promise<typeof fetch> {

return typeof module === 'function' ? module : module.default;
}

let warningsSuppressed = false;

// In Node.js 23 require()ing ESM modules will work, but emit an experimental warning like
// CommonJS module ABC is loading ES Module XYZ using require().
// This function suppresses experimental warnings to avoid those leaking into end users' output.
function suppressExperimentalWarningsIfNecessary() {
const nodeMajorVersion = process.versions.node.split('.').map(Number)[0];
if (nodeMajorVersion >= 23 && !warningsSuppressed) {
const originalEmit = process.emitWarning;
process.emitWarning = (warning, ...args: any[]): void => {
if (args[0] === 'ExperimentalWarning') {
return;
}

if (
typeof args[0] === 'object' &&
args[0].type === 'ExperimentalWarning'
) {
return;
}

originalEmit(warning, ...args);
};

warningsSuppressed = true;
}
}
Copy link
Collaborator

@addaleax addaleax Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the approach we're going with – which I think is pretty reasonable – then I'd do that in the mongosh CLI itself, like we already disable warnings for the case of the compiled executable in packages/cli-repl/src/run.ts, because that's where we're expecting overrides for Node.js behavior and other modifications to truly global state, how does that sound to you?

Copy link
Collaborator Author

@nirinchev nirinchev Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that sounds reasonable to me! I guess originally I wanted to put it next to the code that does the requiring shenanigans, but you're right that the warnings being displayed or not is better left to the user-facing application.

Copy link
Collaborator

@addaleax addaleax Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think there's also just going to be more code running into this problem in the future, the more popular ESM gets in our dependencies, not just the code here


let cachedFetch: Promise<typeof fetch> | undefined;

export type { Request, Response, RequestInfo, RequestInit };
Expand Down
Loading