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
Merged

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

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
DRY
  • Loading branch information
@aduh95
aduh95 committed Jul 24, 2023
commit 56a8a07c7088bcf011a910c9adb1e18cf6e15794
26 changes: 11 additions & 15 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 @@ -44,17 +44,21 @@ class ResolveCache extends SafeMap {
',');
Comment thread
aduh95 marked this conversation as resolved.
}

#getInternalCache(parentURL) {
Comment thread
aduh95 marked this conversation as resolved.
Outdated
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?

}
return internalCache;
}

/**
* @param {string} serializedKey
* @param {string} parentURL
* @returns {import('./loader').ModuleExports | Promise<import('./loader').ModuleExports>}
*/
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];
}

/**
Expand All @@ -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);
}
}

Expand Down