Skip to content

Commit

Permalink
Apply suggestions from code reviews
Browse files Browse the repository at this point in the history
  • Loading branch information
@aduh95
aduh95 committed Jul 21, 2023
1 parent 3bf7e02 commit 9f36bf7
Show file tree
Hide file tree
Showing 3 changed files with 109 additions and 59 deletions.
47 changes: 21 additions & 26 deletions lib/internal/modules/esm/loader.js
View file
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`
*/
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
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.
* 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<string,string>} importAssertions
* @returns {{
* serializedModuleRequest: string,
* internalCache: Record<string, Promise<import('./loader').ModuleExports> | 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<string, string>} importAttributes
* @returns {import('./loader').ModuleExports | Promise<import('./loader').ModuleExports>}
*/
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<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();
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
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.
{
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

0 comments on commit 9f36bf7

Please sign in to comment.