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 review 
Co-authored-by: Geoffrey Booth <webadmin@geoffreybooth.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
  • Loading branch information
@aduh95 @GeoffreyBooth @ljharb
3 people authored Jul 25, 2023
commit abf105a85e8d9ad7ac9e5b88aff17e6fc3e9f36d
20 changes: 10 additions & 10 deletions lib/internal/modules/esm/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion lib/internal/modules/esm/module_job.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
9 changes: 5 additions & 4 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 @@ -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.
*/
Comment thread
aduh95 marked this conversation as resolved.
class ResolveCache extends SafeMap {
constructor(i) { super(i); } // eslint-disable-line no-useless-constructor
Expand All @@ -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(
Expand All @@ -44,7 +45,7 @@ class ResolveCache extends SafeMap {
',');
Comment thread
aduh95 marked this conversation as resolved.
}

#getInternalCache(parentURL) {
#getModuleCachedImports(parentURL) {
let internalCache = super.get(parentURL);
if (internalCache == null) {
super.set(parentURL, internalCache = { __proto__: null });
Copy link
Copy Markdown
Member

@GeoffreyBooth GeoffreyBooth Jul 25, 2023

Choose a reason for hiding this comment

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

Suggested change
super.set(parentURL, internalCache = { __proto__: null });
super.set(parentURL, internalCache = new SafeMap());

And associated changes.

Copy link
Copy Markdown
Contributor Author

@aduh95 aduh95 Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not going to do that in this PR, if Map is faster than an object, we should update LoadCache as well and that's out of scope for this PR.

Copy link
Copy Markdown
Member

@GeoffreyBooth GeoffreyBooth Jul 25, 2023

Choose a reason for hiding this comment

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

Fair enough. @JakobJingleheimer if you still do a refactor after this, maybe add this to the list of improvements?

Expand Down