Skip to content

module: ensure successful dynamic import returns the same result #46662

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

Merged
nodejs-github-bot merged 13 commits into from
Jul 26, 2023
+255 −27
Merged

module: ensure successful dynamic import returns the same result #46662

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0f1fe00
module: ensure successful import returns the same result
aduh95 Apr 14, 2023
3bf7e02
create `ModuleResolveMap` class
aduh95 Jun 10, 2023
9f36bf7
Apply suggestions from code reviews
aduh95 Jul 2, 2023
aabe393
Make the resolve cache a resolve cache
aduh95 Jul 21, 2023
be69edd
revert changes in `test/es-module/test-esm-initialization.mjs`
aduh95 Jul 21, 2023
dd06864
fix missing object
aduh95 Jul 22, 2023
631d7c6
add benchmark
aduh95 Jul 22, 2023
c279d18
add `data:` URL to benchmark
aduh95 Jul 22, 2023
56a8a07
DRY
aduh95 Jul 24, 2023
abf105a
Apply suggestions from code review
aduh95 Jul 25, 2023
00ea14c
fix lint
aduh95 Jul 25, 2023
a791dc9
Update lib/internal/modules/esm/module_map.js
aduh95 Jul 26, 2023
3638858
fix lint
aduh95 Jul 26, 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
Prev Previous commit
Next Next commit
Apply suggestions from code reviews
  • Loading branch information
@aduh95
aduh95 committed Jul 21, 2023
commit 9f36bf78d83bb59e83c95acfe65b5bc64cbb3a00
47 changes: 21 additions & 26 deletions lib/internal/modules/esm/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -71,7 +71,7 @@ class ModuleLoader {
/**
* Import cache
*/
#resolveCache = newModuleResolveMap();
#resolveCache = newResolveCache();

/**
* Map of already-loaded CJS modules to use
Expand All @@ -86,7 +86,7 @@ class ModuleLoader {
/**
* Registry of loaded modules, akin to `require.cache`
Comment thread
aduh95 marked this conversation as resolved.
Outdated
*/
moduleMap = newModuleLoadMap();
moduleMap = newLoadCache();

/**
* Methods which translate input code or other information into ES modules
Expand Down Expand Up @@ -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());
}

/**
Expand All @@ -308,48 +308,43 @@ class ModuleLoader {
* @returns {Promise<ModuleExports>}
*/
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) {
Expand Down
101 changes: 78 additions & 23 deletions lib/internal/modules/esm/module_map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment thread
aduh95 marked this conversation as resolved.
* 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.
*/
Comment thread
aduh95 marked this conversation as resolved.
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<string,string>} importAssertions
* @returns {{
* serializedModuleRequest: string,
* internalCache: Record<string, Promise<import('./loader').ModuleExports> | import('./loader').ModuleExports>,
Copy link
Copy Markdown
Contributor

@guybedford guybedford Jul 17, 2023

Choose a reason for hiding this comment

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

It might be worth entirely separating the serialization process from the lookup process?

Something like:

const requestKey = resolveCache.serializeKey(specifier, attributes);
const absoluteResolvedURL = await resolve(specifier, parentURL, attributes);
const resolvedKey = resolveCache.serializeKey(absoluteResolvedURL, attributes);
resolveCache.set(requestKey, parentKey, resolvedKey);

Ideally we should also be using parentKey not parentURL I think. And then note that you probably want to avoid repeat serialization so you want to be maintaining these keys on the module jobs themselves for easy lookup.

Does that make sense? Let me know if I can clarify any of the above.

Copy link
Copy Markdown
Contributor Author

@aduh95 aduh95 Jul 17, 2023

Choose a reason for hiding this comment

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

Ideally we should also be using parentKey not parentURL I think

That's a good remark, I didn't consider this. I don't think we have access to the parentKey when doing dynamic imports, do we? 🤔 Using parentURL doesn't go against the spec IIUC, it does add a limitation that's not in the spec (but it likely won't matter that much in practice, I'd expect most use case would be more than fine if modules that have the same URL use the same resolve cache), so I'm tempted to say parentURL is good enough. wdyt?

It might be worth entirely separating the serialization process from the lookup process?

That's probably a good idea. The current implementation is probably not going to stay as Jacob is trying to refactor all that is his PR, so I'm hesitant on spending too much time crafting an implementation that we want to get rid of anyway. What I'm mostly interested in is to get the tests to land, so we can refactor with more confidence.

Copy link
Copy Markdown
Member

@GeoffreyBooth GeoffreyBooth Jul 17, 2023

Choose a reason for hiding this comment

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

What is parentKey exactly? A serialized parentURL plus assertions/attributes?

* }}
*/
getSerialized(specifier, parentURL, importAssertions) {
Comment thread
aduh95 marked this conversation as resolved.
Outdated
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
Comment thread
aduh95 marked this conversation as resolved.
Outdated
// as identical.
Comment thread
aduh95 marked this conversation as resolved.
Outdated
const serializedModuleRequest = specifier + ArrayPrototypeJoin(
ArrayPrototypeMap(
ArrayPrototypeSort(ObjectKeys(importAssertions)),
(key) => JSONStringify(key) + JSONStringify(importAssertions[key])),
',');
Comment thread
aduh95 marked this conversation as resolved.

return { specifierCache, serializedAttributes };
return { internalCache, serializedModuleRequest };
}

/**
* @param {string} specifier
* @param {string} parentURL
* @param {Record<string, string>} importAttributes
* @returns {import('./loader').ModuleExports | Promise<import('./loader').ModuleExports>}
*/
get(specifier, parentURL, importAttributes) {
Copy link
Copy Markdown
Member

@GeoffreyBooth GeoffreyBooth Jul 16, 2023

Choose a reason for hiding this comment

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

Let’s please not mix and match importAssertions and importAttributes. We should do another PR that goes and replaces assertions with attributes, and not only renames things but also updates whatever behaviors need to change as part of revising to use the new spec. Unless you want to do that first, though, in this PR I think we should stick to importAssertions.

Copy link
Copy Markdown
Contributor Author

@aduh95 aduh95 Jul 17, 2023

Choose a reason for hiding this comment

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

I'm using the updated spec though, this is following what import attributes are, not import assertions. The inconsistency is regretable, but it would still be inconsistent to call the variable importAssertions, I don't really mind either way.

Copy link
Copy Markdown
Member

@GeoffreyBooth GeoffreyBooth Jul 17, 2023

Choose a reason for hiding this comment

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

What would be different here if you weren't following the new spec?

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<string, string>} importAttributes
* @param {import('./loader').ModuleExports | Promise<import('./loader').ModuleExports>} 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<string, string>} 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();
Copy link
Copy Markdown
Contributor

@guybedford guybedford Jul 17, 2023

Choose a reason for hiding this comment

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

It should be possible to avoid this indirection by having a double-level cache - firstly making this explicitly a resolve cache (String -> AbsoluteURLString) and then secondly looking up the resolve cache string in the global registry.

The global registry itself likely will want to be keyed by the serialized absolute module anyway too in future.

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');
Expand Down Expand Up @@ -89,6 +144,6 @@ class ModuleLoadMap extends SafeMap {
}

module.exports = {
ModuleLoadMap,
ModuleResolveMap,
LoadCache,
ResolveCache,
};
20 changes: 10 additions & 10 deletions test/es-module/test-esm-loader-modulemap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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.
Comment thread
aduh95 marked this conversation as resolved.
{
const moduleMap = new ModuleLoadMap();
const moduleMap = new LoadCache();

moduleMap.set(jsModuleDataUrl, undefined, jsModuleJob);
moduleMap.set(jsonModuleDataUrl, 'json', jsonModuleJob);
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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), {
Expand Down