Skip to content

Commit

Permalink
esm: fallback to getSource when load returns nullish source
Browse files Browse the repository at this point in the history
When using the Modules Customization Hooks API to load CommonJS modules,
we want to support the returned value of `defaultLoad` which must be
nullish to preserve backward compatibility. This can be achieved by
fetching the source from the translator.

PR-URL: #50825
Fixes: #50435
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
  • Loading branch information
aduh95 authored and UlisesGascon committed Dec 19, 2023
1 parent 8d5469c commit 843d5f8
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 12 deletions.
1 change: 1 addition & 0 deletions lib/internal/modules/esm/load.js
Original file line number Diff line number Diff line change
Expand Up @@ -262,5 +262,6 @@ function throwUnknownModuleFormat(url, format) {
module.exports = {
defaultLoad,
defaultLoadSync,
getSourceSync,
throwUnknownModuleFormat,
};
35 changes: 23 additions & 12 deletions lib/internal/modules/esm/translators.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ function lazyTypes() {
}

const { containsModuleSyntax } = internalBinding('contextify');
const { BuiltinModule } = require('internal/bootstrap/realm');
const assert = require('internal/assert');
const { readFileSync } = require('fs');
const { dirname, extname, isAbsolute } = require('path');
Expand Down Expand Up @@ -58,6 +59,17 @@ const asyncESM = require('internal/process/esm_loader');
const { emitWarningSync } = require('internal/process/warning');
const { internalCompileFunction } = require('internal/vm');

// Lazy-loading to avoid circular dependencies.
let getSourceSync;
/**
* @param {Parameters<typeof import('./load').getSourceSync>[0]} url
* @returns {ReturnType<typeof import('./load').getSourceSync>}
*/
function getSource(url) {
getSourceSync ??= require('internal/modules/esm/load').getSourceSync;
return getSourceSync(url);
}

/** @type {import('deps/cjs-module-lexer/lexer.js').parse} */
let cjsParse;
/**
Expand Down Expand Up @@ -225,21 +237,19 @@ function loadCJSModule(module, source, url, filename) {
// eslint-disable-next-line func-name-matching,func-style
const requireFn = function require(specifier) {
let importAttributes = kEmptyObject;
if (!StringPrototypeStartsWith(specifier, 'node:')) {
if (!StringPrototypeStartsWith(specifier, 'node:') && !BuiltinModule.normalizeRequirableId(specifier)) {
// TODO: do not depend on the monkey-patchable CJS loader here.
const path = CJSModule._resolveFilename(specifier, module);
if (specifier !== path) {
switch (extname(path)) {
case '.json':
importAttributes = { __proto__: null, type: 'json' };
break;
case '.node':
return CJSModule._load(specifier, module);
default:
switch (extname(path)) {
case '.json':
importAttributes = { __proto__: null, type: 'json' };
break;
case '.node':
return CJSModule._load(specifier, module);
default:
// fall through
}
specifier = `${pathToFileURL(path)}`;
}
specifier = `${pathToFileURL(path)}`;
}
const job = asyncESM.esmLoader.getModuleJobSync(specifier, url, importAttributes);
job.runSync();
Expand Down Expand Up @@ -276,7 +286,8 @@ function createCJSModuleWrap(url, source, isMain, loadCJS = loadCJSModule) {
debug(`Translating CJSModule ${url}`);

const filename = StringPrototypeStartsWith(url, 'file://') ? fileURLToPath(url) : url;
source = stringify(source);
// In case the source was not provided by the `load` step, we need fetch it now.
source = stringify(source ?? getSource(new URL(url)).source);

const { exportNames, module } = cjsPreparseModuleExports(filename, source);
cjsCache.set(url, module);
Expand Down
35 changes: 35 additions & 0 deletions test/es-module/test-esm-loader-hooks.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -736,4 +736,39 @@ describe('Loader hooks', { concurrency: true }, () => {
assert.strictEqual(code, 0);
assert.strictEqual(signal, null);
});

it('should handle mixed of opt-in modules and non-opt-in ones', async () => {
const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [
'--no-warnings',
'--experimental-loader',
`data:text/javascript,const fixtures=${JSON.stringify(fixtures.path('empty.js'))};export ${
encodeURIComponent(function resolve(s, c, n) {
if (s.endsWith('entry-point')) {
return {
shortCircuit: true,
url: 'file:///c:/virtual-entry-point',
format: 'commonjs',
};
}
return n(s, c);
})
}export ${
encodeURIComponent(async function load(u, c, n) {
if (u === 'file:///c:/virtual-entry-point') {
return {
shortCircuit: true,
source: `"use strict";require(${JSON.stringify(fixtures)});console.log("Hello");`,
format: 'commonjs',
};
}
return n(u, c);
})}`,
'entry-point',
]);

assert.strictEqual(stderr, '');
assert.strictEqual(stdout, 'Hello\n');
assert.strictEqual(code, 0);
assert.strictEqual(signal, null);
});
});

0 comments on commit 843d5f8

Please sign in to comment.