Skip to content

esm: do not call `getSource` when format is `commonjs`

Rodrigo Muino Tomonari requested to merge github/fork/fasttime/fix-defaultLoad into main

This PR ensures that defaultLoad does not access the file system to read the source of modules that are known in advance to be in CommonJS format. This is not necessary, since the CommonJS loader can't use the source property and must instead read the file once again.

For context, this was noticed while investigating a test failure in ESLint on Node.js 21.1.0. Our logic (now fixed) was incorrectly assuming that some asynchronous operations would complete in a specific order, and this order has changed from Node.js 21.0.0 to 21.1.0. The simplest repro I've found is this one:

// index.mjs
await import('./file1.cjs');
process.nextTick(() => console.log('Tick'));
await import('./file2.cjs');
console.log('Done');

with file1.cjs and file2.cjs both empty modules.

This is the output of running index.mjs in Node.js 21.0.0:

Done
Tick

And this is the output in Node.js 21.1.0:

Tick
Done

Here is another similar repro in a single file:

import { mkdtemp, writeFile }   from 'node:fs/promises';
import { join }                 from 'node:path';
import { pathToFileURL }        from 'node:url';

const dir = await mkdtemp('test');
const file1 = join(dir, 'file1.cjs');
const file2 = join(dir, 'file2.cjs');
await Promise.all([writeFile(file1, ''), await writeFile(file2, '')]);

await import(pathToFileURL(file1));
process.nextTick(() => console.log('Tick'));
await import(pathToFileURL(file2));
console.log('Done');

The reason for the inverted order is this commit and a later change, which is causing getSource to be called even when the format of a module is already known to be commonjs, for example because of the .cjs extension. getSource accesses the file system and that consistently requires one or more I/O cycles to complete. This allows callbacks scheduled with process.nextTick to run in the meantime.

Clearly, importing a CommonJS module is an asynchronous operation which souldn't be assumed to resolve in a specific order or number of phases, but calling getSource for known CommonJS modules isn't needed and should be avoided to not occupy the event loop unnecessarily.

Refs: https://github.com/eslint/eslint/pull/17683

Merge request reports