Skip to content
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

module: make CJS load from ESM loader #47999

Merged
merged 32 commits into from
Aug 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
3deeccf
module: make CJS load from ESM loader
aduh95 May 11, 2023
c1b346c
fixup! module: make CJS load from ESM loader
aduh95 May 14, 2023
df14ab8
fixup! module: make CJS load from ESM loader
aduh95 May 14, 2023
f5fdea1
make CJS ModuleWrap instanciation consistent
aduh95 May 23, 2023
598d789
fixup! make CJS ModuleWrap instanciation consistent
aduh95 May 23, 2023
afd882a
add some properties to `require`
aduh95 May 23, 2023
9958f1f
fixup! add some properties to `require`
aduh95 May 23, 2023
6d57350
fixes
aduh95 May 25, 2023
ad7a8ed
add support for requiring absolute URLs, JSON files, and `.node`
aduh95 May 25, 2023
d0e4abf
fix `CustomizedModuleLoader`
aduh95 May 25, 2023
39219be
stringify at the right places, `process.mainModule`
aduh95 May 25, 2023
739dc9e
add dynamic import in CJS + enriched errors
aduh95 May 31, 2023
297a63b
fix rebase conflict
aduh95 Jul 29, 2023
32bf878
fix cache duplication issues
aduh95 Jul 29, 2023
74c93d1
make the change opt-in
aduh95 Jul 30, 2023
0a63101
fix failing tests
aduh95 Jul 30, 2023
a6f01a3
update docs
aduh95 Jul 31, 2023
3b39ba3
style
aduh95 Jul 31, 2023
dd2af79
already addressed TODO
aduh95 Jul 31, 2023
f692715
Apply suggestions from code review
aduh95 Jul 31, 2023
850831b
fixes
aduh95 Jul 31, 2023
c7edb23
add tests
aduh95 Jul 31, 2023
2a5521e
Apply suggestions from code review
aduh95 Jul 31, 2023
507cd21
fixes
aduh95 Jul 31, 2023
ec6d178
fix typo
aduh95 Jul 31, 2023
279e19d
nits
aduh95 Aug 2, 2023
454985b
fix rebase conflicts
aduh95 Aug 3, 2023
2c499bc
Apply suggestions from code review
aduh95 Aug 7, 2023
2c5fcfb
Apply suggestions from code review
aduh95 Aug 7, 2023
8291086
Update lib/internal/modules/esm/loader.js
aduh95 Aug 7, 2023
624d933
do not make the test depend on how many require calls there is in `co…
aduh95 Aug 12, 2023
5d9cdd0
fixup! do not make the test depend on how many require calls there is…
aduh95 Aug 13, 2023
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
50 changes: 38 additions & 12 deletions doc/api/esm.md
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,9 @@ export function resolve(specifier, context, nextResolve) {

<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/47999
description: Add support for `source` with format `commonjs`.
- version:
- v18.6.0
- v16.17.0
Expand Down Expand Up @@ -945,20 +948,43 @@ validating the import assertion.

The final value of `format` must be one of the following:

| `format` | Description | Acceptable types for `source` returned by `load` |
| ------------ | ------------------------------ | ----------------------------------------------------- |
| `'builtin'` | Load a Node.js builtin module | Not applicable |
| `'commonjs'` | Load a Node.js CommonJS module | Not applicable |
| `'json'` | Load a JSON file | { [`string`][], [`ArrayBuffer`][], [`TypedArray`][] } |
| `'module'` | Load an ES module | { [`string`][], [`ArrayBuffer`][], [`TypedArray`][] } |
| `'wasm'` | Load a WebAssembly module | { [`ArrayBuffer`][], [`TypedArray`][] } |
| `format` | Description | Acceptable types for `source` returned by `load` |
| ------------ | ------------------------------ | -------------------------------------------------------------------------- |
| `'builtin'` | Load a Node.js builtin module | Not applicable |
| `'commonjs'` | Load a Node.js CommonJS module | { [`string`][], [`ArrayBuffer`][], [`TypedArray`][], `null`, `undefined` } |
| `'json'` | Load a JSON file | { [`string`][], [`ArrayBuffer`][], [`TypedArray`][] } |
| `'module'` | Load an ES module | { [`string`][], [`ArrayBuffer`][], [`TypedArray`][] } |
| `'wasm'` | Load a WebAssembly module | { [`ArrayBuffer`][], [`TypedArray`][] } |

The value of `source` is ignored for type `'builtin'` because currently it is
not possible to replace the value of a Node.js builtin (core) module. The value
of `source` is ignored for type `'commonjs'` because the CommonJS module loader
does not provide a mechanism for the ES module loader to override the
[CommonJS module return value](#commonjs-namespaces). This limitation might be
overcome in the future.
not possible to replace the value of a Node.js builtin (core) module.

The value of `source` can be omitted for type `'commonjs'`. When a `source` is
provided, all `require` calls from this module will be processed by the ESM
loader with registered `resolve` and `load` hooks; all `require.resolve` calls
from this module will be processed by the ESM loader with registered `resolve`
hooks; `require.extensions` and monkey-patching on the CommonJS module loader
will not apply. If `source` is undefined or `null`, it will be handled by the
CommonJS module loader and `require`/`require.resolve` calls will not go through
the registered hooks. This behavior for nullish `source` is temporary — in the
future, nullish `source` will not be supported.

The Node.js own `load` implementation, which is the value of `next` for the last
loader in the `load` chain, returns `null` for `source` when `format` is
`'commonjs'` for backward compatibility. Here is an example loader that would
opt-in to using the non-default behavior:

```js
import { readFile } from 'node:fs/promises';

export async function load(url, context, nextLoad) {
const result = await nextLoad(url, context);
if (result.format === 'commonjs') {
result.source ??= await readFile(new URL(result.responseURL ?? url));
}
return result;
}
```

> **Caveat**: The ESM `load` hook and namespaced exports from CommonJS modules
> are incompatible. Attempting to use them together will result in an empty
Expand Down
74 changes: 72 additions & 2 deletions lib/internal/modules/esm/load.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const { kEmptyObject } = require('internal/util');
const { defaultGetFormat } = require('internal/modules/esm/get_format');
const { validateAssertions } = require('internal/modules/esm/assert');
const { getOptionValue } = require('internal/options');
const { readFileSync } = require('fs');

// Do not eagerly grab .manifest, it may be in TDZ
const policy = getOptionValue('--experimental-policy') ?
Expand Down Expand Up @@ -69,12 +70,35 @@ async function getSource(url, context) {
return { __proto__: null, responseURL, source };
}

function getSourceSync(url, context) {
const parsed = new URL(url);
const responseURL = url;
let source;
if (parsed.protocol === 'file:') {
source = readFileSync(parsed);
} else if (parsed.protocol === 'data:') {
JakobJingleheimer marked this conversation as resolved.
Show resolved Hide resolved
const match = RegExpPrototypeExec(DATA_URL_PATTERN, parsed.pathname);
GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved
if (!match) {
throw new ERR_INVALID_URL(url);
}
const { 1: base64, 2: body } = match;
source = BufferFrom(decodeURIComponent(body), base64 ? 'base64' : 'utf8');
} else {
const supportedSchemes = ['file', 'data'];
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
throw new ERR_UNSUPPORTED_ESM_URL_SCHEME(parsed, supportedSchemes);
}
if (policy?.manifest) {
policy.manifest.assertIntegrity(parsed, source);
}
Comment on lines +90 to +92
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (policy?.manifest) {
policy.manifest.assertIntegrity(parsed, source);
}
policy?.manifest.assertIntegrity(parsed, source);

(possibly might need another ? between manifest and assertIntegrity)

Copy link
Contributor Author

@aduh95 aduh95 Aug 7, 2023

Choose a reason for hiding this comment

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

I copied that from getSource, we should probably address both in a follow up PR.

return { __proto__: null, responseURL, source };
}


/**
* Node.js default load hook.
* @param {string} url
* @param {object} context
* @returns {object}
* @param {LoadContext} context
* @returns {LoadReturn}
*/
async function defaultLoad(url, context = kEmptyObject) {
let responseURL = url;
Expand Down Expand Up @@ -108,6 +132,51 @@ async function defaultLoad(url, context = kEmptyObject) {
source,
};
}
/**
* @typedef LoadContext
* @property {string} [format] A hint (possibly returned from `resolve`)
* @property {string | Buffer | ArrayBuffer} [source] source
* @property {Record<string, string>} [importAssertions] import attributes
*/

/**
* @typedef LoadReturn
* @property {string} format format
* @property {URL['href']} responseURL The module's fully resolved URL
* @property {Buffer} source source
JakobJingleheimer marked this conversation as resolved.
Show resolved Hide resolved
*/

/**
* @param {URL['href']} url
* @param {LoadContext} [context]
* @returns {LoadReturn}
*/
function defaultLoadSync(url, context = kEmptyObject) {
let responseURL = url;
const { importAssertions } = context;
let {
format,
source,
} = context;

format ??= defaultGetFormat(new URL(url), context);
Comment on lines +156 to +162
Copy link
Contributor

Choose a reason for hiding this comment

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

Format does not / should not change, it just needs a default value, so I think it would be better to keep it a constant with a default:

Suggested change
const { importAssertions } = context;
let {
format,
source,
} = context;
format ??= defaultGetFormat(new URL(url), context);
const {
format = defaultGetFormat(new URL(url), context),
importAssertions,
} = context;
let { source } = context;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if format is null?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, we have a bug in the resolve hook final result validation:

if (
format != null &&
typeof format !== 'string' // [2]
) {

format is an enum, so null is not valid (only the enum values or nothing)

Let's fix them together (we can't fix this part without creating a second bug).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure we've always accepted nullish values indifferently in the hook APIs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even for enums? Why? null is an actual value.


validateAssertions(url, format, importAssertions);

if (format === 'builtin') {
source = null;
} else if (source == null) {
({ responseURL, source } = getSourceSync(url, context));
}

return {
__proto__: null,
format,
responseURL,
source,
};
}


/**
* throws an error if the protocol is not one of the protocols
Expand Down Expand Up @@ -160,5 +229,6 @@ function throwUnknownModuleFormat(url, format) {

module.exports = {
defaultLoad,
defaultLoadSync,
throwUnknownModuleFormat,
};
51 changes: 37 additions & 14 deletions lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const {
} = primordials;

const {
ERR_REQUIRE_ESM,
ERR_UNKNOWN_MODULE_FORMAT,
} = require('internal/errors').codes;
const { getOptionValue } = require('internal/options');
Expand All @@ -18,7 +19,7 @@ const { emitExperimentalWarning } = require('internal/util');
const {
getDefaultConditions,
} = require('internal/modules/esm/utils');
let defaultResolve, defaultLoad, importMetaInitializer;
let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer;

function newResolveCache() {
const { ResolveCache } = require('internal/modules/esm/module_map');
Expand Down Expand Up @@ -220,7 +221,12 @@ class ModuleLoader {
return this.getJobFromResolveResult(resolveResult, parentURL, importAssertions);
}

getJobFromResolveResult(resolveResult, parentURL, importAssertions) {
getModuleJobSync(specifier, parentURL, importAssertions) {
const resolveResult = this.resolveSync(specifier, parentURL, importAssertions);
return this.getJobFromResolveResult(resolveResult, parentURL, importAssertions, true);
}

getJobFromResolveResult(resolveResult, parentURL, importAssertions, sync) {
const { url, format } = resolveResult;
const resolvedImportAssertions = resolveResult.importAssertions ?? importAssertions;
let job = this.loadCache.get(url, resolvedImportAssertions.type);
Expand All @@ -231,7 +237,7 @@ class ModuleLoader {
}

if (job === undefined) {
job = this.#createModuleJob(url, resolvedImportAssertions, parentURL, format);
job = this.#createModuleJob(url, resolvedImportAssertions, parentURL, format, sync);
}
aduh95 marked this conversation as resolved.
Show resolved Hide resolved

return job;
Expand All @@ -248,17 +254,8 @@ class ModuleLoader {
* `resolve` hook
* @returns {Promise<ModuleJob>} The (possibly pending) module job
*/
#createModuleJob(url, importAssertions, parentURL, format) {
const moduleProvider = async (url, isMain) => {
const {
format: finalFormat,
responseURL,
source,
} = await this.load(url, {
format,
importAssertions,
});

#createModuleJob(url, importAssertions, parentURL, format, sync) {
const callTranslator = ({ format: finalFormat, responseURL, source }, isMain) => {
const translator = getTranslators().get(finalFormat);

if (!translator) {
Expand All @@ -267,6 +264,10 @@ class ModuleLoader {

return FunctionPrototypeCall(translator, this, responseURL, source, isMain);
};
const context = { format, importAssertions };
const moduleProvider = sync ?
(url, isMain) => callTranslator(this.loadSync(url, context), isMain) :
async (url, isMain) => callTranslator(await this.load(url, context), isMain);

const inspectBrk = (
parentURL === undefined &&
Expand All @@ -285,6 +286,7 @@ class ModuleLoader {
moduleProvider,
parentURL === undefined,
inspectBrk,
sync,
);

this.loadCache.set(url, importAssertions.type, job);
Expand Down Expand Up @@ -388,6 +390,24 @@ class ModuleLoader {
return result;
}

loadSync(url, context) {
defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync;

let result = this.#customizations ?
this.#customizations.loadSync(url, context) :
defaultLoadSync(url, context);
let format = result?.format;
if (format === 'module') {
throw new ERR_REQUIRE_ESM(url, true);
}
if (format === 'commonjs') {
format = 'require-commonjs';
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
result = { __proto__: result, format };
}
this.validateLoadResult(url, format);
return result;
}

validateLoadResult(url, format) {
if (format == null) {
require('internal/modules/esm/load').throwUnknownModuleFormat(url, format);
Expand Down Expand Up @@ -465,6 +485,9 @@ class CustomizedModuleLoader {
load(url, context) {
return hooksProxy.makeAsyncRequest('load', undefined, url, context);
}
loadSync(url, context) {
return hooksProxy.makeSyncRequest('load', undefined, url, context);
}

importMetaInitialize(meta, context, loader) {
hooksProxy.importMetaInitialize(meta, context, loader);
Expand Down
25 changes: 24 additions & 1 deletion lib/internal/modules/esm/module_job.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,26 @@ class ModuleJob {
// `loader` is the Loader instance used for loading dependencies.
// `moduleProvider` is a function
constructor(loader, url, importAssertions = { __proto__: null },
moduleProvider, isMain, inspectBrk) {
moduleProvider, isMain, inspectBrk, sync = false) {
this.loader = loader;
this.importAssertions = importAssertions;
this.isMain = isMain;
this.inspectBrk = inspectBrk;

this.url = url;

this.module = undefined;
// Expose the promise to the ModuleWrap directly for linking below.
// `this.module` is also filled in below.
this.modulePromise = ReflectApply(moduleProvider, loader, [url, isMain]);

if (sync) {
this.module = this.modulePromise;
this.modulePromise = PromiseResolve(this.module);
} else {
this.modulePromise = PromiseResolve(this.modulePromise);
}

// Wait for the ModuleWrap instance being linked with all dependencies.
const link = async () => {
this.module = await this.modulePromise;
Expand Down Expand Up @@ -186,6 +195,20 @@ class ModuleJob {
}
}

runSync() {
assert(this.module instanceof ModuleWrap);
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
if (this.instantiated !== undefined) {
return { __proto__: null, module: this.module };
}

this.module.instantiate();
this.instantiated = PromiseResolve();
const timeout = -1;
JakobJingleheimer marked this conversation as resolved.
Show resolved Hide resolved
const breakOnSigint = false;
this.module.evaluate(timeout, breakOnSigint);
GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved
return { __proto__: null, module: this.module };
}

async run() {
await this.instantiate();
const timeout = -1;
Expand Down
Loading