From 0f1fe00745879f676f314cf33ae722f9fb484080 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Fri, 14 Apr 2023 18:09:49 +0200 Subject: [PATCH 01/13] module: ensure successful import returns the same result --- lib/internal/modules/esm/loader.js | 73 ++++++++++++++++++- lib/internal/modules/esm/module_job.js | 4 +- .../test-esm-dynamic-import-mutating-fs.js | 25 +++++++ .../test-esm-dynamic-import-mutating-fs.mjs | 42 +++++++++++ test/es-module/test-esm-initialization.mjs | 12 +-- 5 files changed, 148 insertions(+), 8 deletions(-) create mode 100644 test/es-module/test-esm-dynamic-import-mutating-fs.js create mode 100644 test/es-module/test-esm-dynamic-import-mutating-fs.mjs diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index d1cc9da3c74baf..03eb68b59b75d0 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -4,8 +4,16 @@ require('internal/modules/cjs/loader'); const { + ArrayPrototypeJoin, + ArrayPrototypeMap, + ArrayPrototypeSort, FunctionPrototypeCall, + JSONStringify, + ObjectKeys, ObjectSetPrototypeOf, + PromisePrototypeThen, + SafeMap, + PromiseResolve, SafeWeakMap, } = primordials; @@ -61,6 +69,11 @@ class ModuleLoader { */ #defaultConditions = getDefaultConditions(); + /** + * Import cache + */ + #importCache = new SafeMap(); + /** * Map of already-loaded CJS modules to use */ @@ -282,6 +295,34 @@ class ModuleLoader { return job; } + #serializeCache(specifier, parentURL, importAssertions) { + let cache = this.#importCache.get(parentURL); + let specifierCache; + if (cache == null) { + this.#importCache.set(parentURL, cache = new SafeMap()); + } else { + specifierCache = cache.get(specifier); + } + + if (specifierCache == null) { + cache.set(specifier, specifierCache = { __proto__: null }); + } + + const serializedAttributes = ArrayPrototypeJoin( + ArrayPrototypeMap( + ArrayPrototypeSort(ObjectKeys(importAssertions)), + (key) => JSONStringify(key) + JSONStringify(importAssertions[key])), + ','); + + return { specifierCache, serializedAttributes }; + } + + cacheStatic(specifier, parentURL, importAssertions, result) { + const { specifierCache, serializedAttributes } = this.#serializeCache(specifier, parentURL, importAssertions); + + specifierCache[serializedAttributes] = result; + } + /** * This method is usually called indirectly as part of the loading processes. * Use directly with caution. @@ -292,8 +333,38 @@ class ModuleLoader { * @returns {Promise} */ async import(specifier, parentURL, importAssertions) { - const moduleJob = await this.getModuleJob(specifier, parentURL, importAssertions); + const { specifierCache, serializedAttributes } = this.#serializeCache(specifier, parentURL, importAssertions); + const removeCache = () => { + delete specifierCache[serializedAttributes]; + }; + if (specifierCache[serializedAttributes] != null) { + if (PromiseResolve(specifierCache[serializedAttributes]) !== specifierCache[serializedAttributes]) { + const { module } = await specifierCache[serializedAttributes].run(); + return module.getNamespace(); + } + const fallback = () => { + if (specifierCache[serializedAttributes] != null) { + return PromisePrototypeThen(specifierCache[serializedAttributes], undefined, fallback); + } + const result = this.#import(specifier, parentURL, importAssertions); + specifierCache[serializedAttributes] = result; + PromisePrototypeThen(result, undefined, removeCache); + return result; + }; + return PromisePrototypeThen(specifierCache[serializedAttributes], undefined, fallback); + } + const result = this.#import(specifier, parentURL, importAssertions); + specifierCache[serializedAttributes] = result; + PromisePrototypeThen(result, undefined, removeCache); + return result; + } + + async #import(specifier, parentURL, importAssertions) { + const moduleJob = this.getModuleJob(specifier, parentURL, importAssertions); const { module } = await moduleJob.run(); + + const { specifierCache, serializedAttributes } = this.#serializeCache(specifier, parentURL, importAssertions); + specifierCache[serializedAttributes] = moduleJob; return module.getNamespace(); } diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index cb1654812f9d82..e58e1f24b29c74 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -75,7 +75,9 @@ class ModuleJob { const promises = this.module.link(async (specifier, assertions) => { const job = await this.loader.getModuleJob(specifier, url, assertions); ArrayPrototypePush(dependencyJobs, job); - return job.modulePromise; + const result = await job.modulePromise; + this.loader.cacheStatic(specifier, url, assertions, job); + return result; }); if (promises !== undefined) diff --git a/test/es-module/test-esm-dynamic-import-mutating-fs.js b/test/es-module/test-esm-dynamic-import-mutating-fs.js new file mode 100644 index 00000000000000..09cbffe487959e --- /dev/null +++ b/test/es-module/test-esm-dynamic-import-mutating-fs.js @@ -0,0 +1,25 @@ +'use strict'; +const common = require('../common'); +const tmpdir = require('../common/tmpdir'); + +const assert = require('node:assert'); +const fs = require('node:fs/promises'); +const { pathToFileURL } = require('node:url'); + +tmpdir.refresh(); +const tmpDir = pathToFileURL(tmpdir.path); + +const target = new URL(`./${Math.random()}.mjs`, tmpDir); + +(async () => { + + await assert.rejects(import(target), { code: 'ERR_MODULE_NOT_FOUND' }); + + await fs.writeFile(target, 'export default "actual target"\n'); + + const moduleRecord = await import(target); + + await fs.rm(target); + + assert.strictEqual(await import(target), moduleRecord); +})().then(common.mustCall()); diff --git a/test/es-module/test-esm-dynamic-import-mutating-fs.mjs b/test/es-module/test-esm-dynamic-import-mutating-fs.mjs new file mode 100644 index 00000000000000..7eb79337065765 --- /dev/null +++ b/test/es-module/test-esm-dynamic-import-mutating-fs.mjs @@ -0,0 +1,42 @@ +import { spawnPromisified } from '../common/index.mjs'; +import tmpdir from '../common/tmpdir.js'; + +import assert from 'node:assert'; +import fs from 'node:fs/promises'; +import { execPath } from 'node:process'; +import { pathToFileURL } from 'node:url'; + +tmpdir.refresh(); +const tmpDir = pathToFileURL(tmpdir.path); + +const target = new URL(`./${Math.random()}.mjs`, tmpDir); + +await assert.rejects(import(target), { code: 'ERR_MODULE_NOT_FOUND' }); + +await fs.writeFile(target, 'export default "actual target"\n'); + +const moduleRecord = await import(target); + +await fs.rm(target); + +assert.strictEqual(await import(target), moduleRecord); + +// Add the file back, it should be deleted by the subprocess. +await fs.writeFile(target, 'export default "actual target"\n'); + +assert.deepStrictEqual( + await spawnPromisified(execPath, [ + '--input-type=module', + '--eval', + [`import * as d from${JSON.stringify(target)};`, + 'import{rm}from"node:fs/promises";', + `await rm(new URL(${JSON.stringify(target)}));`, + 'import{strictEqual}from"node:assert";', + `strictEqual(JSON.stringify(await import(${JSON.stringify(target)})),JSON.stringify(d));`].join(''), + ]), + { + code: 0, + signal: null, + stderr: '', + stdout: '', + }); diff --git a/test/es-module/test-esm-initialization.mjs b/test/es-module/test-esm-initialization.mjs index aa946a50152d40..cbc384b4118978 100644 --- a/test/es-module/test-esm-initialization.mjs +++ b/test/es-module/test-esm-initialization.mjs @@ -8,22 +8,22 @@ import { describe, it } from 'node:test'; describe('ESM: ensure initialization happens only once', { concurrency: true }, () => { it(async () => { const { code, stderr, stdout } = await spawnPromisified(execPath, [ + '--experimental-import-meta-resolve', '--loader', fixtures.fileURL('es-module-loaders', 'loader-resolve-passthru.mjs'), '--no-warnings', fixtures.path('es-modules', 'runmain.mjs'), ]); - // Length minus 1 because the first match is the needle. - const resolveHookRunCount = (stdout.match(/resolve passthru/g)?.length ?? 0) - 1; - assert.strictEqual(stderr, ''); /** - * resolveHookRunCount = 2: - * 1. fixtures/…/runmain.mjs + * 'resolve passthru' appears 4 times in the output: + * 1. fixtures/…/runmain.mjs (entry point) * 2. node:module (imported by fixtures/…/runmain.mjs) + * 3. doesnt-matter.mjs (first import.meta.resolve call) + * 4. doesnt-matter.mjs (second import.meta.resolve call) */ - assert.strictEqual(resolveHookRunCount, 2); + assert.strictEqual(stdout.match(/resolve passthru/g)?.length, 4); assert.strictEqual(code, 0); }); }); From 3bf7e02a90cfb1012607e9530d216b3a9a2c06e5 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sat, 10 Jun 2023 20:12:41 +0200 Subject: [PATCH 02/13] create `ModuleResolveMap` class --- lib/internal/modules/esm/loader.js | 110 +++++++++----------- lib/internal/modules/esm/module_job.js | 2 +- lib/internal/modules/esm/module_map.js | 59 ++++++++++- test/es-module/test-esm-loader-modulemap.js | 20 ++-- 4 files changed, 117 insertions(+), 74 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 03eb68b59b75d0..a9de53f3f2357f 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -4,16 +4,9 @@ require('internal/modules/cjs/loader'); const { - ArrayPrototypeJoin, - ArrayPrototypeMap, - ArrayPrototypeSort, FunctionPrototypeCall, - JSONStringify, - ObjectKeys, ObjectSetPrototypeOf, PromisePrototypeThen, - SafeMap, - PromiseResolve, SafeWeakMap, } = primordials; @@ -23,14 +16,20 @@ const { const { getOptionValue } = require('internal/options'); const { pathToFileURL } = require('internal/url'); const { emitExperimentalWarning } = require('internal/util'); +const { isPromise } = require('internal/util/types'); const { getDefaultConditions, } = require('internal/modules/esm/utils'); let defaultResolve, defaultLoad, importMetaInitializer; -function newModuleMap() { - const ModuleMap = require('internal/modules/esm/module_map'); - return new ModuleMap(); +function newModuleResolveMap() { + const { ModuleResolveMap } = require('internal/modules/esm/module_map'); + return new ModuleResolveMap(); +} + +function newModuleLoadMap() { + const { ModuleLoadMap } = require('internal/modules/esm/module_map'); + return new ModuleLoadMap(); } function getTranslators() { @@ -72,7 +71,7 @@ class ModuleLoader { /** * Import cache */ - #importCache = new SafeMap(); + #resolveCache = newModuleResolveMap(); /** * Map of already-loaded CJS modules to use @@ -87,7 +86,7 @@ class ModuleLoader { /** * Registry of loaded modules, akin to `require.cache` */ - moduleMap = newModuleMap(); + moduleMap = newModuleLoadMap(); /** * Methods which translate input code or other information into ES modules @@ -295,32 +294,8 @@ class ModuleLoader { return job; } - #serializeCache(specifier, parentURL, importAssertions) { - let cache = this.#importCache.get(parentURL); - let specifierCache; - if (cache == null) { - this.#importCache.set(parentURL, cache = new SafeMap()); - } else { - specifierCache = cache.get(specifier); - } - - if (specifierCache == null) { - cache.set(specifier, specifierCache = { __proto__: null }); - } - - const serializedAttributes = ArrayPrototypeJoin( - ArrayPrototypeMap( - ArrayPrototypeSort(ObjectKeys(importAssertions)), - (key) => JSONStringify(key) + JSONStringify(importAssertions[key])), - ','); - - return { specifierCache, serializedAttributes }; - } - - cacheStatic(specifier, parentURL, importAssertions, result) { - const { specifierCache, serializedAttributes } = this.#serializeCache(specifier, parentURL, importAssertions); - - specifierCache[serializedAttributes] = result; + cacheStaticImportResult(specifier, parentURL, importAttributes, job) { + this.#resolveCache.set(specifier, parentURL, importAttributes, () => job.module.getNamespace()); } /** @@ -333,38 +308,55 @@ class ModuleLoader { * @returns {Promise} */ async import(specifier, parentURL, importAssertions) { - const { specifierCache, serializedAttributes } = this.#serializeCache(specifier, parentURL, importAssertions); + const { specifierCache, serializedAttributes } = + this.#resolveCache.getSerialized(specifier, parentURL, importAssertions); const removeCache = () => { + // Remove the cache entry if the import fails. delete specifierCache[serializedAttributes]; }; - if (specifierCache[serializedAttributes] != null) { - if (PromiseResolve(specifierCache[serializedAttributes]) !== specifierCache[serializedAttributes]) { - const { module } = await specifierCache[serializedAttributes].run(); - return module.getNamespace(); - } - const fallback = () => { - if (specifierCache[serializedAttributes] != null) { - return PromisePrototypeThen(specifierCache[serializedAttributes], undefined, fallback); - } - const result = this.#import(specifier, parentURL, importAssertions); + + // If there are no cache entry, create one: + if (specifierCache[serializedAttributes] == null) { + const result = this.#import(specifier, parentURL, importAssertions); + // Cache the Promise for now: + specifierCache[serializedAttributes] = result; + PromisePrototypeThen(result, (result) => { + // Once the promise has resolved, we can cache the ModuleJob itself. specifierCache[serializedAttributes] = result; - PromisePrototypeThen(result, undefined, removeCache); - return result; - }; - return PromisePrototypeThen(specifierCache[serializedAttributes], undefined, fallback); + }, removeCache); + return result; } - const result = this.#import(specifier, parentURL, importAssertions); - specifierCache[serializedAttributes] = result; - PromisePrototypeThen(result, undefined, removeCache); - return result; + + // If the cache entry is a function, it's a static import that has already been successfully loaded: + if (typeof specifierCache[serializedAttributes] === 'function') { + return specifierCache[serializedAttributes] = specifierCache[serializedAttributes](); + } + + // If the cached value is not a promise, it's already been successfully loaded: + if (!isPromise(specifierCache[serializedAttributes])) { + return specifierCache[serializedAttributes]; + } + + // If it's still a promise, we must have a fallback in case it fails: + const fallback = () => { + // If another fallback has already cached a promise, use this one: + if (specifierCache[serializedAttributes] != null) { + return PromisePrototypeThen(specifierCache[serializedAttributes], undefined, fallback); + } + // Otherwise create a new cache entry: + const result = this.#import(specifier, parentURL, importAssertions); + specifierCache[serializedAttributes] = result; + PromisePrototypeThen(result, undefined, removeCache); + return result; + }; + return PromisePrototypeThen(specifierCache[serializedAttributes], undefined, fallback); } async #import(specifier, parentURL, importAssertions) { const moduleJob = this.getModuleJob(specifier, parentURL, importAssertions); const { module } = await moduleJob.run(); - const { specifierCache, serializedAttributes } = this.#serializeCache(specifier, parentURL, importAssertions); - specifierCache[serializedAttributes] = moduleJob; + this.#resolveCache.set(specifier, parentURL, importAssertions, moduleJob); return module.getNamespace(); } diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index e58e1f24b29c74..bbda3316eeaedd 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -76,7 +76,7 @@ class ModuleJob { const job = await this.loader.getModuleJob(specifier, url, assertions); ArrayPrototypePush(dependencyJobs, job); const result = await job.modulePromise; - this.loader.cacheStatic(specifier, url, assertions, job); + this.loader.cacheStaticImportResult(specifier, url, attributes, job); return result; }); diff --git a/lib/internal/modules/esm/module_map.js b/lib/internal/modules/esm/module_map.js index ac6d95445ae757..0f92afc42a9194 100644 --- a/lib/internal/modules/esm/module_map.js +++ b/lib/internal/modules/esm/module_map.js @@ -1,17 +1,64 @@ 'use strict'; -const { kImplicitAssertType } = require('internal/modules/esm/assert'); const { + ArrayPrototypeJoin, + ArrayPrototypeMap, + ArrayPrototypeSort, + JSONStringify, + ObjectKeys, SafeMap, } = primordials; +const { kImplicitAssertType } = require('internal/modules/esm/assert'); let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { debug = fn; }); const { ERR_INVALID_ARG_TYPE } = require('internal/errors').codes; const { validateString } = require('internal/validators'); +class ModuleResolveMap extends SafeMap { + constructor(i) { super(i); } // eslint-disable-line no-useless-constructor + + getSerialized(specifier, parentURL, importAssertions) { + let cache = super.get(parentURL); + let specifierCache; + if (cache == null) { + super.set(parentURL, cache = new SafeMap()); + } else { + specifierCache = cache.get(specifier); + } + + if (specifierCache == null) { + cache.set(specifier, specifierCache = { __proto__: null }); + } + + const serializedAttributes = ArrayPrototypeJoin( + ArrayPrototypeMap( + ArrayPrototypeSort(ObjectKeys(importAssertions)), + (key) => JSONStringify(key) + JSONStringify(importAssertions[key])), + ','); + + return { specifierCache, serializedAttributes }; + } + + get(specifier, parentURL, importAttributes) { + const { specifierCache, serializedAttributes } = this.getSerialized(specifier, parentURL, importAttributes); + return specifierCache[serializedAttributes]; + } + + set(specifier, parentURL, importAttributes, job) { + const { specifierCache, serializedAttributes } = this.getSerialized(specifier, parentURL, importAttributes); + specifierCache[serializedAttributes] = job; + return this; + } + + has(specifier, parentURL, importAttributes) { + const { specifierCache, serializedAttributes } = this.getSerialized(specifier, parentURL, importAttributes); + return serializedAttributes in specifierCache; + } +} + // Tracks the state of the loader-level module cache -class ModuleMap extends SafeMap { +class ModuleLoadMap extends SafeMap { constructor(i) { super(i); } // eslint-disable-line no-useless-constructor get(url, type = kImplicitAssertType) { validateString(url, 'url'); @@ -29,7 +76,7 @@ class ModuleMap extends SafeMap { } debug(`Storing ${url} (${ type === kImplicitAssertType ? 'implicit type' : type - }) in ModuleMap`); + }) in ModuleLoadMap`); const cachedJobsForUrl = super.get(url) ?? { __proto__: null }; cachedJobsForUrl[type] = job; return super.set(url, cachedJobsForUrl); @@ -40,4 +87,8 @@ class ModuleMap extends SafeMap { return super.get(url)?.[type] !== undefined; } } -module.exports = ModuleMap; + +module.exports = { + ModuleLoadMap, + ModuleResolveMap, +}; diff --git a/test/es-module/test-esm-loader-modulemap.js b/test/es-module/test-esm-loader-modulemap.js index 093a4f6b3ef162..09fa0f8c40cb07 100644 --- a/test/es-module/test-esm-loader-modulemap.js +++ b/test/es-module/test-esm-loader-modulemap.js @@ -5,7 +5,7 @@ require('../common'); const { strictEqual, throws } = require('assert'); const { createModuleLoader } = require('internal/modules/esm/loader'); -const ModuleMap = require('internal/modules/esm/module_map'); +const { ModuleLoadMap } = require('internal/modules/esm/module_map'); const ModuleJob = require('internal/modules/esm/module_job'); const createDynamicModule = require( 'internal/modules/esm/create_dynamic_module'); @@ -24,11 +24,11 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module, () => new Promise(() => {})); -// ModuleMap.set and ModuleMap.get store and retrieve module jobs for a -// specified url/type tuple; ModuleMap.has correctly reports whether such jobs +// ModuleLoadMap.set and ModuleLoadMap.get store and retrieve module jobs for a +// specified url/type tuple; ModuleLoadMap.has correctly reports whether such jobs // are stored in the map. { - const moduleMap = new ModuleMap(); + const moduleMap = new ModuleLoadMap(); moduleMap.set(jsModuleDataUrl, undefined, jsModuleJob); moduleMap.set(jsonModuleDataUrl, 'json', jsonModuleJob); @@ -50,10 +50,10 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module, strictEqual(moduleMap.has(jsonModuleDataUrl, 'unknown'), false); } -// ModuleMap.get, ModuleMap.has and ModuleMap.set should only accept string +// ModuleLoadMap.get, ModuleLoadMap.has and ModuleLoadMap.set should only accept string // values as url argument. { - const moduleMap = new ModuleMap(); + const moduleMap = new ModuleLoadMap(); const errorObj = { code: 'ERR_INVALID_ARG_TYPE', @@ -68,10 +68,10 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module, }); } -// ModuleMap.get, ModuleMap.has and ModuleMap.set should only accept string +// ModuleLoadMap.get, ModuleLoadMap.has and ModuleLoadMap.set should only accept string // values (or the kAssertType symbol) as type argument. { - const moduleMap = new ModuleMap(); + const moduleMap = new ModuleLoadMap(); const errorObj = { code: 'ERR_INVALID_ARG_TYPE', @@ -86,9 +86,9 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module, }); } -// ModuleMap.set should only accept ModuleJob values as job argument. +// ModuleLoadMap.set should only accept ModuleJob values as job argument. { - const moduleMap = new ModuleMap(); + const moduleMap = new ModuleLoadMap(); [{}, [], true, 1].forEach((value) => { throws(() => moduleMap.set('', undefined, value), { From 9f36bf78d83bb59e83c95acfe65b5bc64cbb3a00 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sun, 2 Jul 2023 15:46:49 +0200 Subject: [PATCH 03/13] Apply suggestions from code reviews --- lib/internal/modules/esm/loader.js | 47 ++++----- lib/internal/modules/esm/module_map.js | 101 +++++++++++++++----- test/es-module/test-esm-loader-modulemap.js | 20 ++-- 3 files changed, 109 insertions(+), 59 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index a9de53f3f2357f..46297fdb2aa611 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -22,14 +22,14 @@ const { } = require('internal/modules/esm/utils'); let defaultResolve, defaultLoad, importMetaInitializer; -function newModuleResolveMap() { - const { ModuleResolveMap } = require('internal/modules/esm/module_map'); - return new ModuleResolveMap(); +function newResolveCache() { + const { ResolveCache } = require('internal/modules/esm/module_map'); + return new ResolveCache(); } -function newModuleLoadMap() { - const { ModuleLoadMap } = require('internal/modules/esm/module_map'); - return new ModuleLoadMap(); +function newLoadCache() { + const { LoadCache } = require('internal/modules/esm/module_map'); + return new LoadCache(); } function getTranslators() { @@ -71,7 +71,7 @@ class ModuleLoader { /** * Import cache */ - #resolveCache = newModuleResolveMap(); + #resolveCache = newResolveCache(); /** * Map of already-loaded CJS modules to use @@ -86,7 +86,7 @@ class ModuleLoader { /** * Registry of loaded modules, akin to `require.cache` */ - moduleMap = newModuleLoadMap(); + moduleMap = newLoadCache(); /** * Methods which translate input code or other information into ES modules @@ -295,7 +295,7 @@ class ModuleLoader { } cacheStaticImportResult(specifier, parentURL, importAttributes, job) { - this.#resolveCache.set(specifier, parentURL, importAttributes, () => job.module.getNamespace()); + this.#resolveCache.setLazy(specifier, parentURL, importAttributes, () => job.module.getNamespace()); } /** @@ -308,48 +308,43 @@ class ModuleLoader { * @returns {Promise} */ async import(specifier, parentURL, importAssertions) { - const { specifierCache, serializedAttributes } = + const { internalCache, serializedModuleRequest } = this.#resolveCache.getSerialized(specifier, parentURL, importAssertions); const removeCache = () => { // Remove the cache entry if the import fails. - delete specifierCache[serializedAttributes]; + delete internalCache[serializedModuleRequest]; }; - // If there are no cache entry, create one: - if (specifierCache[serializedAttributes] == null) { + // If there is no entry in the cache for this import, create one: + if (internalCache[serializedModuleRequest] == null) { const result = this.#import(specifier, parentURL, importAssertions); // Cache the Promise for now: - specifierCache[serializedAttributes] = result; + internalCache[serializedModuleRequest] = result; PromisePrototypeThen(result, (result) => { // Once the promise has resolved, we can cache the ModuleJob itself. - specifierCache[serializedAttributes] = result; + internalCache[serializedModuleRequest] = result; }, removeCache); return result; } - // If the cache entry is a function, it's a static import that has already been successfully loaded: - if (typeof specifierCache[serializedAttributes] === 'function') { - return specifierCache[serializedAttributes] = specifierCache[serializedAttributes](); - } - // If the cached value is not a promise, it's already been successfully loaded: - if (!isPromise(specifierCache[serializedAttributes])) { - return specifierCache[serializedAttributes]; + if (!isPromise(internalCache[serializedModuleRequest])) { + return internalCache[serializedModuleRequest]; } // If it's still a promise, we must have a fallback in case it fails: const fallback = () => { // If another fallback has already cached a promise, use this one: - if (specifierCache[serializedAttributes] != null) { - return PromisePrototypeThen(specifierCache[serializedAttributes], undefined, fallback); + if (internalCache[serializedModuleRequest] != null) { + return PromisePrototypeThen(internalCache[serializedModuleRequest], undefined, fallback); } // Otherwise create a new cache entry: const result = this.#import(specifier, parentURL, importAssertions); - specifierCache[serializedAttributes] = result; + internalCache[serializedModuleRequest] = result; PromisePrototypeThen(result, undefined, removeCache); return result; }; - return PromisePrototypeThen(specifierCache[serializedAttributes], undefined, fallback); + return PromisePrototypeThen(internalCache[serializedModuleRequest], undefined, fallback); } async #import(specifier, parentURL, importAssertions) { diff --git a/lib/internal/modules/esm/module_map.js b/lib/internal/modules/esm/module_map.js index 0f92afc42a9194..167b3caa114aaf 100644 --- a/lib/internal/modules/esm/module_map.js +++ b/lib/internal/modules/esm/module_map.js @@ -5,60 +5,115 @@ const { ArrayPrototypeMap, ArrayPrototypeSort, JSONStringify, + ObjectDefineProperty, ObjectKeys, SafeMap, } = primordials; const { kImplicitAssertType } = require('internal/modules/esm/assert'); +const { setOwnProperty } = require('internal/util'); let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { debug = fn; }); const { ERR_INVALID_ARG_TYPE } = require('internal/errors').codes; const { validateString } = require('internal/validators'); -class ModuleResolveMap extends SafeMap { +/** + * Cache the results of the `resolve` step of the module resolution and loading process. + * Future resolutions of the same input (specifier, parent URL and import assertions) + * must return the same result if the first attempt was successful, per + * https://tc39.es/ecma262/#sec-HostLoadImportedModule. + */ +class ResolveCache extends SafeMap { constructor(i) { super(i); } // eslint-disable-line no-useless-constructor + /** + * Generates the internal serialized cache key and returns it along the actual cache object. + * + * It is exposed to allow more efficient read and overwrite a cache entry. + * @param {string} specifier + * @param {string} parentURL + * @param {Record} importAssertions + * @returns {{ + * serializedModuleRequest: string, + * internalCache: Record | import('./loader').ModuleExports>, + * }} + */ getSerialized(specifier, parentURL, importAssertions) { - let cache = super.get(parentURL); - let specifierCache; - if (cache == null) { - super.set(parentURL, cache = new SafeMap()); - } else { - specifierCache = cache.get(specifier); + let internalCache = super.get(parentURL); + if (internalCache == null) { + super.set(parentURL, internalCache = { __proto__: null }); } - if (specifierCache == null) { - cache.set(specifier, specifierCache = { __proto__: null }); - } - - const serializedAttributes = ArrayPrototypeJoin( + // To serialize the ModuleRequest (specifier + list of import attributes), + // we need to sort the attributes by key, then stringifying, + // so that different import statements with the same attributes are always treated + // as identical. + const serializedModuleRequest = specifier + ArrayPrototypeJoin( ArrayPrototypeMap( ArrayPrototypeSort(ObjectKeys(importAssertions)), (key) => JSONStringify(key) + JSONStringify(importAssertions[key])), ','); - return { specifierCache, serializedAttributes }; + return { internalCache, serializedModuleRequest }; } + /** + * @param {string} specifier + * @param {string} parentURL + * @param {Record} importAttributes + * @returns {import('./loader').ModuleExports | Promise} + */ get(specifier, parentURL, importAttributes) { - const { specifierCache, serializedAttributes } = this.getSerialized(specifier, parentURL, importAttributes); - return specifierCache[serializedAttributes]; + const { internalCache, serializedModuleRequest } = this.getSerialized(specifier, parentURL, importAttributes); + return internalCache[serializedModuleRequest]; } + /** + * @param {string} specifier + * @param {string} parentURL + * @param {Record} importAttributes + * @param {import('./loader').ModuleExports | Promise} job + */ set(specifier, parentURL, importAttributes, job) { - const { specifierCache, serializedAttributes } = this.getSerialized(specifier, parentURL, importAttributes); - specifierCache[serializedAttributes] = job; + const { internalCache, serializedModuleRequest } = this.getSerialized(specifier, parentURL, importAttributes); + internalCache[serializedModuleRequest] = job; + return this; + } + + /** + * Adds a cache entry that won't be evaluated until the first time someone tries to read it. + * + * Static imports need to be lazily cached as the link step is done before the + * module exports are actually available. + * @param {string} specifier + * @param {string} parentURL + * @param {Record} importAttributes + * @param {() => import('./loader').ModuleExports} getJob + */ + setLazy(specifier, parentURL, importAttributes, getJob) { + const { internalCache, serializedModuleRequest } = this.getSerialized(specifier, parentURL, importAttributes); + ObjectDefineProperty(internalCache, serializedModuleRequest, { + __proto__: null, + configurable: true, + get() { + const val = getJob(); + setOwnProperty(internalCache, serializedModuleRequest, val); + return val; + }, + }); return this; } has(specifier, parentURL, importAttributes) { - const { specifierCache, serializedAttributes } = this.getSerialized(specifier, parentURL, importAttributes); - return serializedAttributes in specifierCache; + const { internalCache, serializedModuleRequest } = this.getSerialized(specifier, parentURL, importAttributes); + return serializedModuleRequest in internalCache; } } -// Tracks the state of the loader-level module cache -class ModuleLoadMap extends SafeMap { +/** + * Cache the results of the `load` step of the module resolution and loading process. + */ +class LoadCache extends SafeMap { constructor(i) { super(i); } // eslint-disable-line no-useless-constructor get(url, type = kImplicitAssertType) { validateString(url, 'url'); @@ -89,6 +144,6 @@ class ModuleLoadMap extends SafeMap { } module.exports = { - ModuleLoadMap, - ModuleResolveMap, + LoadCache, + ResolveCache, }; diff --git a/test/es-module/test-esm-loader-modulemap.js b/test/es-module/test-esm-loader-modulemap.js index 09fa0f8c40cb07..95e998adde335f 100644 --- a/test/es-module/test-esm-loader-modulemap.js +++ b/test/es-module/test-esm-loader-modulemap.js @@ -5,7 +5,7 @@ require('../common'); const { strictEqual, throws } = require('assert'); const { createModuleLoader } = require('internal/modules/esm/loader'); -const { ModuleLoadMap } = require('internal/modules/esm/module_map'); +const { LoadCache } = require('internal/modules/esm/module_map'); const ModuleJob = require('internal/modules/esm/module_job'); const createDynamicModule = require( 'internal/modules/esm/create_dynamic_module'); @@ -24,11 +24,11 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module, () => new Promise(() => {})); -// ModuleLoadMap.set and ModuleLoadMap.get store and retrieve module jobs for a -// specified url/type tuple; ModuleLoadMap.has correctly reports whether such jobs +// LoadCache.set and LoadCache.get store and retrieve module jobs for a +// specified url/type tuple; LoadCache.has correctly reports whether such jobs // are stored in the map. { - const moduleMap = new ModuleLoadMap(); + const moduleMap = new LoadCache(); moduleMap.set(jsModuleDataUrl, undefined, jsModuleJob); moduleMap.set(jsonModuleDataUrl, 'json', jsonModuleJob); @@ -50,10 +50,10 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module, strictEqual(moduleMap.has(jsonModuleDataUrl, 'unknown'), false); } -// ModuleLoadMap.get, ModuleLoadMap.has and ModuleLoadMap.set should only accept string +// LoadCache.get, LoadCache.has and LoadCache.set should only accept string // values as url argument. { - const moduleMap = new ModuleLoadMap(); + const moduleMap = new LoadCache(); const errorObj = { code: 'ERR_INVALID_ARG_TYPE', @@ -68,10 +68,10 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module, }); } -// ModuleLoadMap.get, ModuleLoadMap.has and ModuleLoadMap.set should only accept string +// LoadCache.get, LoadCache.has and LoadCache.set should only accept string // values (or the kAssertType symbol) as type argument. { - const moduleMap = new ModuleLoadMap(); + const moduleMap = new LoadCache(); const errorObj = { code: 'ERR_INVALID_ARG_TYPE', @@ -86,9 +86,9 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module, }); } -// ModuleLoadMap.set should only accept ModuleJob values as job argument. +// LoadCache.set should only accept ModuleJob values as job argument. { - const moduleMap = new ModuleLoadMap(); + const moduleMap = new LoadCache(); [{}, [], true, 1].forEach((value) => { throws(() => moduleMap.set('', undefined, value), { From aabe393c137ef4b7b645039e7dd93327553146a6 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Fri, 21 Jul 2023 21:19:36 +0200 Subject: [PATCH 04/13] Make the resolve cache a resolve cache --- lib/internal/modules/esm/loader.js | 91 ++++++--------------- lib/internal/modules/esm/module_job.js | 4 +- lib/internal/modules/esm/module_map.js | 78 ++++++------------ test/es-module/test-esm-loader-modulemap.js | 20 ++++- 4 files changed, 69 insertions(+), 124 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 46297fdb2aa611..1f97363b1f9860 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -6,7 +6,6 @@ require('internal/modules/cjs/loader'); const { FunctionPrototypeCall, ObjectSetPrototypeOf, - PromisePrototypeThen, SafeWeakMap, } = primordials; @@ -16,22 +15,21 @@ const { const { getOptionValue } = require('internal/options'); const { pathToFileURL } = require('internal/url'); const { emitExperimentalWarning } = require('internal/util'); -const { isPromise } = require('internal/util/types'); const { getDefaultConditions, } = require('internal/modules/esm/utils'); let defaultResolve, defaultLoad, importMetaInitializer; -function newResolveCache() { - const { ResolveCache } = require('internal/modules/esm/module_map'); - return new ResolveCache(); -} - function newLoadCache() { const { LoadCache } = require('internal/modules/esm/module_map'); return new LoadCache(); } +function newResolveCache() { + const { ResolveCache } = require('internal/modules/esm/module_map'); + return new ResolveCache(); +} + function getTranslators() { const { translators } = require('internal/modules/esm/translators'); return translators; @@ -68,11 +66,6 @@ class ModuleLoader { */ #defaultConditions = getDefaultConditions(); - /** - * Import cache - */ - #resolveCache = newResolveCache(); - /** * Map of already-loaded CJS modules to use */ @@ -86,7 +79,12 @@ class ModuleLoader { /** * Registry of loaded modules, akin to `require.cache` */ - moduleMap = newLoadCache(); + loadCache = newLoadCache(); + + /** + * Registry of resolved URLs + */ + #resolveCache = newResolveCache(); /** * Methods which translate input code or other information into ES modules @@ -195,7 +193,7 @@ class ModuleLoader { const ModuleJob = require('internal/modules/esm/module_job'); const job = new ModuleJob( this, url, undefined, evalInstance, false, false); - this.moduleMap.set(url, undefined, job); + this.loadCache.set(url, undefined, job); const { module } = await job.run(); return { @@ -225,11 +223,11 @@ class ModuleLoader { getJobFromResolveResult(resolveResult, parentURL, importAssertions) { const { url, format } = resolveResult; const resolvedImportAssertions = resolveResult.importAssertions ?? importAssertions; - let job = this.moduleMap.get(url, resolvedImportAssertions.type); + let job = this.loadCache.get(url, resolvedImportAssertions.type); // CommonJS will set functions for lazy job evaluation. if (typeof job === 'function') { - this.moduleMap.set(url, undefined, job = job()); + this.loadCache.set(url, undefined, job = job()); } if (job === undefined) { @@ -289,15 +287,11 @@ class ModuleLoader { inspectBrk, ); - this.moduleMap.set(url, importAssertions.type, job); + this.loadCache.set(url, importAssertions.type, job); return job; } - cacheStaticImportResult(specifier, parentURL, importAttributes, job) { - this.#resolveCache.setLazy(specifier, parentURL, importAttributes, () => job.module.getNamespace()); - } - /** * This method is usually called indirectly as part of the loading processes. * Use directly with caution. @@ -308,50 +302,8 @@ class ModuleLoader { * @returns {Promise} */ async import(specifier, parentURL, importAssertions) { - const { internalCache, serializedModuleRequest } = - this.#resolveCache.getSerialized(specifier, parentURL, importAssertions); - const removeCache = () => { - // Remove the cache entry if the import fails. - delete internalCache[serializedModuleRequest]; - }; - - // If there is no entry in the cache for this import, create one: - if (internalCache[serializedModuleRequest] == null) { - const result = this.#import(specifier, parentURL, importAssertions); - // Cache the Promise for now: - internalCache[serializedModuleRequest] = result; - PromisePrototypeThen(result, (result) => { - // Once the promise has resolved, we can cache the ModuleJob itself. - internalCache[serializedModuleRequest] = result; - }, removeCache); - return result; - } - - // If the cached value is not a promise, it's already been successfully loaded: - if (!isPromise(internalCache[serializedModuleRequest])) { - return internalCache[serializedModuleRequest]; - } - - // If it's still a promise, we must have a fallback in case it fails: - const fallback = () => { - // If another fallback has already cached a promise, use this one: - if (internalCache[serializedModuleRequest] != null) { - return PromisePrototypeThen(internalCache[serializedModuleRequest], undefined, fallback); - } - // Otherwise create a new cache entry: - const result = this.#import(specifier, parentURL, importAssertions); - internalCache[serializedModuleRequest] = result; - PromisePrototypeThen(result, undefined, removeCache); - return result; - }; - return PromisePrototypeThen(internalCache[serializedModuleRequest], undefined, fallback); - } - - async #import(specifier, parentURL, importAssertions) { - const moduleJob = this.getModuleJob(specifier, parentURL, importAssertions); + const moduleJob = await this.getModuleJob(specifier, parentURL, importAssertions); const { module } = await moduleJob.run(); - - this.#resolveCache.set(specifier, parentURL, importAssertions, moduleJob); return module.getNamespace(); } @@ -373,13 +325,20 @@ class ModuleLoader { * @param {string} [parentURL] The URL path of the module's parent. * @param {ImportAssertions} importAssertions Assertions from the import * statement or expression. - * @returns {Promise<{ format: string, url: URL['href'] }>} + * @returns {{ format: string, url: URL['href'] }} */ resolve(originalSpecifier, parentURL, importAssertions) { if (this.#customizations) { return this.#customizations.resolve(originalSpecifier, parentURL, importAssertions); } - return this.defaultResolve(originalSpecifier, parentURL, importAssertions); + const requestKey = this.#resolveCache.serializeKey(originalSpecifier, importAssertions); + const cachedResult = this.#resolveCache.get(requestKey, parentURL); + if (cachedResult != null) { + return cachedResult; + } + const result = this.defaultResolve(originalSpecifier, parentURL, importAssertions); + this.#resolveCache.set(requestKey, parentURL, result); + return result; } /** diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index bbda3316eeaedd..cb1654812f9d82 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -75,9 +75,7 @@ class ModuleJob { const promises = this.module.link(async (specifier, assertions) => { const job = await this.loader.getModuleJob(specifier, url, assertions); ArrayPrototypePush(dependencyJobs, job); - const result = await job.modulePromise; - this.loader.cacheStaticImportResult(specifier, url, attributes, job); - return result; + return job.modulePromise; }); if (promises !== undefined) diff --git a/lib/internal/modules/esm/module_map.js b/lib/internal/modules/esm/module_map.js index 167b3caa114aaf..56a58dbac1483f 100644 --- a/lib/internal/modules/esm/module_map.js +++ b/lib/internal/modules/esm/module_map.js @@ -5,12 +5,10 @@ const { ArrayPrototypeMap, ArrayPrototypeSort, JSONStringify, - ObjectDefineProperty, ObjectKeys, SafeMap, } = primordials; const { kImplicitAssertType } = require('internal/modules/esm/assert'); -const { setOwnProperty } = require('internal/util'); let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { debug = fn; }); @@ -31,82 +29,54 @@ class ResolveCache extends SafeMap { * * It is exposed to allow more efficient read and overwrite a cache entry. * @param {string} specifier - * @param {string} parentURL * @param {Record} importAssertions - * @returns {{ - * serializedModuleRequest: string, - * internalCache: Record | import('./loader').ModuleExports>, - * }} + * @returns {string} */ - getSerialized(specifier, parentURL, importAssertions) { - let internalCache = super.get(parentURL); - if (internalCache == null) { - super.set(parentURL, internalCache = { __proto__: null }); - } - + serializeKey(specifier, importAssertions) { // To serialize the ModuleRequest (specifier + list of import attributes), // we need to sort the attributes by key, then stringifying, // so that different import statements with the same attributes are always treated // as identical. - const serializedModuleRequest = specifier + ArrayPrototypeJoin( + return specifier + '::' + ArrayPrototypeJoin( ArrayPrototypeMap( ArrayPrototypeSort(ObjectKeys(importAssertions)), (key) => JSONStringify(key) + JSONStringify(importAssertions[key])), ','); - - return { internalCache, serializedModuleRequest }; } /** - * @param {string} specifier + * @param {string} serializedKey * @param {string} parentURL - * @param {Record} importAttributes * @returns {import('./loader').ModuleExports | Promise} */ - get(specifier, parentURL, importAttributes) { - const { internalCache, serializedModuleRequest } = this.getSerialized(specifier, parentURL, importAttributes); - return internalCache[serializedModuleRequest]; - } - - /** - * @param {string} specifier - * @param {string} parentURL - * @param {Record} importAttributes - * @param {import('./loader').ModuleExports | Promise} job - */ - set(specifier, parentURL, importAttributes, job) { - const { internalCache, serializedModuleRequest } = this.getSerialized(specifier, parentURL, importAttributes); - internalCache[serializedModuleRequest] = job; - return this; + get(serializedKey, parentURL) { + let internalCache = super.get(parentURL); + if (internalCache == null) { + super.set(parentURL, internalCache = { __proto__: null }); + } + return internalCache[serializedKey]; } /** - * Adds a cache entry that won't be evaluated until the first time someone tries to read it. - * - * Static imports need to be lazily cached as the link step is done before the - * module exports are actually available. - * @param {string} specifier + * @param {string} serializedKey * @param {string} parentURL - * @param {Record} importAttributes - * @param {() => import('./loader').ModuleExports} getJob + * @param {{ format: string, url: URL['href'] }} result */ - setLazy(specifier, parentURL, importAttributes, getJob) { - const { internalCache, serializedModuleRequest } = this.getSerialized(specifier, parentURL, importAttributes); - ObjectDefineProperty(internalCache, serializedModuleRequest, { - __proto__: null, - configurable: true, - get() { - const val = getJob(); - setOwnProperty(internalCache, serializedModuleRequest, val); - return val; - }, - }); + set(serializedKey, parentURL, result) { + let internalCache = super.get(parentURL); + if (internalCache == null) { + super.set(parentURL, internalCache = { __proto__: null }); + } + internalCache[serializedKey] = result; return this; } - has(specifier, parentURL, importAttributes) { - const { internalCache, serializedModuleRequest } = this.getSerialized(specifier, parentURL, importAttributes); - return serializedModuleRequest in internalCache; + has(serializedKey, parentURL) { + let internalCache = super.get(parentURL); + if (internalCache == null) { + super.set(parentURL, internalCache = { __proto__: null }); + } + return serializedKey in internalCache; } } diff --git a/test/es-module/test-esm-loader-modulemap.js b/test/es-module/test-esm-loader-modulemap.js index 95e998adde335f..860775df0a2ce8 100644 --- a/test/es-module/test-esm-loader-modulemap.js +++ b/test/es-module/test-esm-loader-modulemap.js @@ -5,7 +5,7 @@ require('../common'); const { strictEqual, throws } = require('assert'); const { createModuleLoader } = require('internal/modules/esm/loader'); -const { LoadCache } = require('internal/modules/esm/module_map'); +const { LoadCache, ResolveCache } = require('internal/modules/esm/module_map'); const ModuleJob = require('internal/modules/esm/module_job'); const createDynamicModule = require( 'internal/modules/esm/create_dynamic_module'); @@ -98,3 +98,21 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module, }); }); } + +{ + const resolveMap = new ResolveCache(); + + strictEqual(resolveMap.serializeKey('./file', { __proto__: null }), './file::'); + strictEqual(resolveMap.serializeKey('./file', { __proto__: null, type: 'json' }), './file::"type""json"'); + strictEqual(resolveMap.serializeKey('./file::"type""json"', { __proto__: null }), './file::"type""json"::'); + strictEqual(resolveMap.serializeKey('./file', { __proto__: null, c: 'd', a: 'b' }), './file::"a""b","c""d"'); + strictEqual(resolveMap.serializeKey('./s', { __proto__: null, c: 'd', a: 'b', b: 'c' }), './s::"a""b","b""c","c""d"'); + + resolveMap.set('key1', 'parent1', 1); + resolveMap.set('key2', 'parent1', 2); + resolveMap.set('key2', 'parent2', 3); + + strictEqual(resolveMap.get('key1', 'parent1'), 1); + strictEqual(resolveMap.get('key2', 'parent1'), 2); + strictEqual(resolveMap.get('key2', 'parent2'), 3); +} From be69edd61c86c15ac14dcd98c9f5675c12c58dc3 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Fri, 21 Jul 2023 21:34:17 +0200 Subject: [PATCH 05/13] revert changes in `test/es-module/test-esm-initialization.mjs` --- test/es-module/test-esm-initialization.mjs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/es-module/test-esm-initialization.mjs b/test/es-module/test-esm-initialization.mjs index cbc384b4118978..aa946a50152d40 100644 --- a/test/es-module/test-esm-initialization.mjs +++ b/test/es-module/test-esm-initialization.mjs @@ -8,22 +8,22 @@ import { describe, it } from 'node:test'; describe('ESM: ensure initialization happens only once', { concurrency: true }, () => { it(async () => { const { code, stderr, stdout } = await spawnPromisified(execPath, [ - '--experimental-import-meta-resolve', '--loader', fixtures.fileURL('es-module-loaders', 'loader-resolve-passthru.mjs'), '--no-warnings', fixtures.path('es-modules', 'runmain.mjs'), ]); + // Length minus 1 because the first match is the needle. + const resolveHookRunCount = (stdout.match(/resolve passthru/g)?.length ?? 0) - 1; + assert.strictEqual(stderr, ''); /** - * 'resolve passthru' appears 4 times in the output: - * 1. fixtures/…/runmain.mjs (entry point) + * resolveHookRunCount = 2: + * 1. fixtures/…/runmain.mjs * 2. node:module (imported by fixtures/…/runmain.mjs) - * 3. doesnt-matter.mjs (first import.meta.resolve call) - * 4. doesnt-matter.mjs (second import.meta.resolve call) */ - assert.strictEqual(stdout.match(/resolve passthru/g)?.length, 4); + assert.strictEqual(resolveHookRunCount, 2); assert.strictEqual(code, 0); }); }); From dd06864e2030e0690f0175da6f54d760edafb84b Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sat, 22 Jul 2023 12:16:01 +0200 Subject: [PATCH 06/13] fix missing object --- lib/internal/modules/esm/module_job.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index cb1654812f9d82..b277d33ae7be80 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -21,7 +21,7 @@ const { const { ModuleWrap } = internalBinding('module_wrap'); -const { decorateErrorStack } = require('internal/util'); +const { decorateErrorStack, kEmptyObject } = require('internal/util'); const { getSourceMapsEnabled, } = require('internal/source_map/source_map_cache'); @@ -140,7 +140,7 @@ class ModuleJob { /module '(.*)' does not provide an export named '(.+)'/, e.message); const { url: childFileURL } = await this.loader.resolve( - childSpecifier, parentFileUrl, + childSpecifier, parentFileUrl, kEmptyObject, ); let format; try { From 631d7c6058896684a2d005a21f9b3fade3f7778f Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sat, 22 Jul 2023 23:04:17 +0200 Subject: [PATCH 07/13] add benchmark --- benchmark/esm/esm-loader-import.js | 44 ++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 benchmark/esm/esm-loader-import.js diff --git a/benchmark/esm/esm-loader-import.js b/benchmark/esm/esm-loader-import.js new file mode 100644 index 00000000000000..f67a6d2ced83a9 --- /dev/null +++ b/benchmark/esm/esm-loader-import.js @@ -0,0 +1,44 @@ +// Tests the impact on eager operations required for policies affecting +// general startup, does not test lazy operations +'use strict'; +const fs = require('node:fs'); +const path = require('node:path'); +const common = require('../common.js'); + +const tmpdir = require('../../test/common/tmpdir.js'); +const { pathToFileURL } = require('node:url'); + +const benchmarkDirectory = pathToFileURL(path.resolve(tmpdir.path, 'benchmark-import')); + +const configs = { + n: [1e3], + specifier: [ + './relative-existing.js', + './relative-nonexistent.js', + 'node:prefixed-nonexistent', + 'node:os', + ], +}; + +const options = { + flags: ['--expose-internals'], +}; + +const bench = common.createBenchmark(main, configs, options); + +async function main(conf) { + tmpdir.refresh(); + + fs.mkdirSync(benchmarkDirectory, { recursive: true }); + fs.writeFileSync(new URL('./relative-existing.js', benchmarkDirectory), '\n'); + + bench.start(); + + for (let i = 0; i < conf.n; i++) { + try { + await import(new URL(conf.specifier, benchmarkDirectory)); + } catch { /* empty */ } + } + + bench.end(conf.n); +} From c279d181de1cb82f9515bd6b09f596a35d1096db Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sun, 23 Jul 2023 01:51:05 +0200 Subject: [PATCH 08/13] add `data:` URL to benchmark --- benchmark/esm/esm-loader-import.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/benchmark/esm/esm-loader-import.js b/benchmark/esm/esm-loader-import.js index f67a6d2ced83a9..9967cd95275469 100644 --- a/benchmark/esm/esm-loader-import.js +++ b/benchmark/esm/esm-loader-import.js @@ -13,6 +13,7 @@ const benchmarkDirectory = pathToFileURL(path.resolve(tmpdir.path, 'benchmark-im const configs = { n: [1e3], specifier: [ + 'data:text/javascript,{i}', './relative-existing.js', './relative-nonexistent.js', 'node:prefixed-nonexistent', @@ -36,7 +37,7 @@ async function main(conf) { for (let i = 0; i < conf.n; i++) { try { - await import(new URL(conf.specifier, benchmarkDirectory)); + await import(new URL(conf.specifier.replace('{i}', i), benchmarkDirectory)); } catch { /* empty */ } } From 56a8a07c7088bcf011a910c9adb1e18cf6e15794 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Mon, 24 Jul 2023 09:39:26 +0200 Subject: [PATCH 09/13] DRY --- lib/internal/modules/esm/module_map.js | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/lib/internal/modules/esm/module_map.js b/lib/internal/modules/esm/module_map.js index 56a58dbac1483f..0057be3c450d6c 100644 --- a/lib/internal/modules/esm/module_map.js +++ b/lib/internal/modules/esm/module_map.js @@ -44,17 +44,21 @@ class ResolveCache extends SafeMap { ','); } + #getInternalCache(parentURL) { + let internalCache = super.get(parentURL); + if (internalCache == null) { + super.set(parentURL, internalCache = { __proto__: null }); + } + return internalCache; + } + /** * @param {string} serializedKey * @param {string} parentURL * @returns {import('./loader').ModuleExports | Promise} */ get(serializedKey, parentURL) { - let internalCache = super.get(parentURL); - if (internalCache == null) { - super.set(parentURL, internalCache = { __proto__: null }); - } - return internalCache[serializedKey]; + return this.#getInternalCache(parentURL)[serializedKey]; } /** @@ -63,20 +67,12 @@ class ResolveCache extends SafeMap { * @param {{ format: string, url: URL['href'] }} result */ set(serializedKey, parentURL, result) { - let internalCache = super.get(parentURL); - if (internalCache == null) { - super.set(parentURL, internalCache = { __proto__: null }); - } - internalCache[serializedKey] = result; + this.#getInternalCache(parentURL)[serializedKey] = result; return this; } has(serializedKey, parentURL) { - let internalCache = super.get(parentURL); - if (internalCache == null) { - super.set(parentURL, internalCache = { __proto__: null }); - } - return serializedKey in internalCache; + return serializedKey in this.#getInternalCache(parentURL); } } From abf105a85e8d9ad7ac9e5b88aff17e6fc3e9f36d Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Tue, 25 Jul 2023 17:29:16 +0200 Subject: [PATCH 10/13] Apply suggestions from code review Co-authored-by: Geoffrey Booth Co-authored-by: Jordan Harband --- lib/internal/modules/esm/loader.js | 20 ++++++++++---------- lib/internal/modules/esm/module_job.js | 4 +++- lib/internal/modules/esm/module_map.js | 9 +++++---- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 1f97363b1f9860..68bd8bdda3eeb6 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -20,16 +20,16 @@ const { } = require('internal/modules/esm/utils'); let defaultResolve, defaultLoad, importMetaInitializer; -function newLoadCache() { - const { LoadCache } = require('internal/modules/esm/module_map'); - return new LoadCache(); -} - function newResolveCache() { - const { ResolveCache } = require('internal/modules/esm/module_map'); + const { ResolveCache } = require(‘internal/modules/esm/module_map’); return new ResolveCache(); } +function newLoadCache() { + const { LoadCache } = require(‘internal/modules/esm/module_map’); + return new LoadCache(); +} + function getTranslators() { const { translators } = require('internal/modules/esm/translators'); return translators; @@ -77,14 +77,14 @@ class ModuleLoader { evalIndex = 0; /** - * Registry of loaded modules, akin to `require.cache` + * Registry of resolved specifiers */ - loadCache = newLoadCache(); + #resolveCache = newResolveCache(); /** - * Registry of resolved URLs + * Registry of loaded modules, akin to `require.cache` */ - #resolveCache = newResolveCache(); + loadCache = newLoadCache(); /** * Methods which translate input code or other information into ES modules diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index b277d33ae7be80..35185c64a7d08f 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -140,7 +140,9 @@ class ModuleJob { /module '(.*)' does not provide an export named '(.+)'/, e.message); const { url: childFileURL } = await this.loader.resolve( - childSpecifier, parentFileUrl, kEmptyObject, + childSpecifier, + parentFileUrl, + kEmptyObject, ); let format; try { diff --git a/lib/internal/modules/esm/module_map.js b/lib/internal/modules/esm/module_map.js index 0057be3c450d6c..a1fb12e3d9e2cc 100644 --- a/lib/internal/modules/esm/module_map.js +++ b/lib/internal/modules/esm/module_map.js @@ -20,6 +20,7 @@ const { validateString } = require('internal/validators'); * Future resolutions of the same input (specifier, parent URL and import assertions) * must return the same result if the first attempt was successful, per * https://tc39.es/ecma262/#sec-HostLoadImportedModule. + * This cache is *not* used when custom loaders are registered. */ class ResolveCache extends SafeMap { constructor(i) { super(i); } // eslint-disable-line no-useless-constructor @@ -33,9 +34,9 @@ class ResolveCache extends SafeMap { * @returns {string} */ serializeKey(specifier, importAssertions) { - // To serialize the ModuleRequest (specifier + list of import attributes), - // we need to sort the attributes by key, then stringifying, - // so that different import statements with the same attributes are always treated + // To serialize the ModuleRequest (specifier + list of import assertions), + // we need to sort the assertions by key, then stringifying, + // so that different import statements with the same assertions are always treated // as identical. return specifier + '::' + ArrayPrototypeJoin( ArrayPrototypeMap( @@ -44,7 +45,7 @@ class ResolveCache extends SafeMap { ','); } - #getInternalCache(parentURL) { + #getModuleCachedImports(parentURL) { let internalCache = super.get(parentURL); if (internalCache == null) { super.set(parentURL, internalCache = { __proto__: null }); From 00ea14c5fd7a04464c0ef46766efb984274dafd7 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Tue, 25 Jul 2023 17:43:53 +0200 Subject: [PATCH 11/13] fix lint --- lib/internal/modules/esm/loader.js | 4 ++-- lib/internal/modules/esm/module_map.js | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 68bd8bdda3eeb6..93ab847bf979d0 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -21,12 +21,12 @@ const { let defaultResolve, defaultLoad, importMetaInitializer; function newResolveCache() { - const { ResolveCache } = require(‘internal/modules/esm/module_map’); + const { ResolveCache } = require('internal/modules/esm/module_map'); return new ResolveCache(); } function newLoadCache() { - const { LoadCache } = require(‘internal/modules/esm/module_map’); + const { LoadCache } = require('internal/modules/esm/module_map'); return new LoadCache(); } diff --git a/lib/internal/modules/esm/module_map.js b/lib/internal/modules/esm/module_map.js index a1fb12e3d9e2cc..15001f61a9826d 100644 --- a/lib/internal/modules/esm/module_map.js +++ b/lib/internal/modules/esm/module_map.js @@ -59,7 +59,7 @@ class ResolveCache extends SafeMap { * @returns {import('./loader').ModuleExports | Promise} */ get(serializedKey, parentURL) { - return this.#getInternalCache(parentURL)[serializedKey]; + return this.#getModuleCachedImports(parentURL)[serializedKey]; } /** @@ -68,12 +68,12 @@ class ResolveCache extends SafeMap { * @param {{ format: string, url: URL['href'] }} result */ set(serializedKey, parentURL, result) { - this.#getInternalCache(parentURL)[serializedKey] = result; + this.#getModuleCachedImports(parentURL)[serializedKey] = result; return this; } has(serializedKey, parentURL) { - return serializedKey in this.#getInternalCache(parentURL); + return serializedKey in this.#getModuleCachedImports(parentURL); } } From a791dc901546a9e352da8387d13f81b5ea0be97a Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 26 Jul 2023 19:07:13 +0200 Subject: [PATCH 12/13] Update lib/internal/modules/esm/module_map.js Co-authored-by: Geoffrey Booth --- lib/internal/modules/esm/module_map.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/internal/modules/esm/module_map.js b/lib/internal/modules/esm/module_map.js index 15001f61a9826d..906358e317198a 100644 --- a/lib/internal/modules/esm/module_map.js +++ b/lib/internal/modules/esm/module_map.js @@ -38,9 +38,15 @@ class ResolveCache extends SafeMap { // we need to sort the assertions by key, then stringifying, // so that different import statements with the same assertions are always treated // as identical. + const keys = ObjectKeys(importAssertions); + + if (keys.length === 0) { + return specifier + '::'; + } + return specifier + '::' + ArrayPrototypeJoin( ArrayPrototypeMap( - ArrayPrototypeSort(ObjectKeys(importAssertions)), + ArrayPrototypeSort(keys), (key) => JSONStringify(key) + JSONStringify(importAssertions[key])), ','); } From 3638858a03b86898381c042f8dcf4815e0682e9b Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 26 Jul 2023 19:21:40 +0200 Subject: [PATCH 13/13] fix lint --- lib/internal/modules/esm/module_map.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/modules/esm/module_map.js b/lib/internal/modules/esm/module_map.js index 906358e317198a..12a1a526178a7e 100644 --- a/lib/internal/modules/esm/module_map.js +++ b/lib/internal/modules/esm/module_map.js @@ -39,7 +39,7 @@ class ResolveCache extends SafeMap { // so that different import statements with the same assertions are always treated // as identical. const keys = ObjectKeys(importAssertions); - + if (keys.length === 0) { return specifier + '::'; }