Skip to content

Commit

Permalink
module: ensure successful dynamic import returns the same result
Browse files Browse the repository at this point in the history
  • Loading branch information
aduh95 committed Feb 14, 2023
1 parent 1b87cb6 commit 5569c58
Show file tree
Hide file tree
Showing 7 changed files with 101 additions and 10 deletions.
8 changes: 4 additions & 4 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -1147,8 +1147,8 @@ function wrapSafe(filename, content, cjsModuleInstance) {
lineOffset: 0,
importModuleDynamically: async (specifier, _, importAssertions) => {
const cascadedLoader = getCascadedLoader();
return cascadedLoader.import(specifier, normalizeReferrerURL(filename),
importAssertions);
return cascadedLoader.dynamicImport(specifier, normalizeReferrerURL(filename),
importAssertions);
},
});

Expand All @@ -1173,8 +1173,8 @@ function wrapSafe(filename, content, cjsModuleInstance) {
filename,
importModuleDynamically(specifier, _, importAssertions) {
const cascadedLoader = getCascadedLoader();
return cascadedLoader.import(specifier, normalizeReferrerURL(filename),
importAssertions);
return cascadedLoader.dynamicImport(specifier, normalizeReferrerURL(filename),
importAssertions);
},
});

Expand Down
46 changes: 44 additions & 2 deletions lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ const {
Array,
ArrayIsArray,
FunctionPrototypeCall,
JSONStringify,
ObjectSetPrototypeOf,
PromisePrototypeThen,
SafeMap,
SafePromiseAllReturnArrayLike,
SafeWeakMap,
} = primordials;
Expand Down Expand Up @@ -71,6 +74,8 @@ class ESMLoader {
#defaultLoad;
#importMetaInitializer;

#dynamicImportCache = new SafeMap();

/**
* The conditions for resolving packages if `--conditions` is not used.
*/
Expand Down Expand Up @@ -124,7 +129,7 @@ class ESMLoader {
const module = new ModuleWrap(url, undefined, source, 0, 0);
setCallbackForWrap(module, {
importModuleDynamically: (specifier, { url }, importAssertions) => {
return this.import(specifier, url, importAssertions);
return this.dynamicImport(specifier, url, importAssertions);
}
});

Expand Down Expand Up @@ -242,6 +247,41 @@ class ESMLoader {
return job;
}

async dynamicImport(specifier, parentURL, importAssertions) {
let cache = this.#dynamicImportCache.get(parentURL);
let specifierCache;
if (cache == null) {
this.#dynamicImportCache.set(parentURL, cache = new SafeMap());
} else {
specifierCache = cache.get(specifier);
}

if (specifierCache == null) {
cache.set(specifier, specifierCache = { __proto__: null });
}

const assertions = JSONStringify(importAssertions);
const removeCache = () => {
delete specifierCache[assertions];
};
if (specifierCache[assertions] != null) {
const fallback = () => {
if (specifierCache[assertions] != null) {
return PromisePrototypeThen(specifierCache[assertions], undefined, fallback);
}
const result = this.import(specifier, parentURL, importAssertions);
specifierCache[assertions] = result;
PromisePrototypeThen(result, undefined, removeCache);
return result;
};
return PromisePrototypeThen(specifierCache[assertions], undefined, fallback);
}
const result = this.import(specifier, parentURL, importAssertions);
specifierCache[assertions] = result;
PromisePrototypeThen(result, undefined, removeCache);
return result;
}

/**
* This method is usually called indirectly as part of the loading processes.
* Internally, it is used directly to add loaders. Use directly with caution.
Expand All @@ -267,7 +307,9 @@ class ESMLoader {
// create redundant work for the caller, so it is later popped off the
// internal list.
const wasArr = ArrayIsArray(specifiers);
if (!wasArr) { specifiers = [specifiers]; }
if (!wasArr) {
specifiers = [specifiers];
}

const count = specifiers.length;
const jobs = new Array(count);
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/modules/esm/translators.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ function errPath(url) {
}

async function importModuleDynamically(specifier, { url }, assertions) {
return asyncESM.esmLoader.import(specifier, url, assertions);
return asyncESM.esmLoader.dynamicImport(specifier, url, assertions);
}

// Strategy for loading a standard JavaScript module.
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/process/execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ function evalScript(name, body, breakFirstLine, print, shouldLoadESM = false) {
[kVmBreakFirstLineSymbol]: !!breakFirstLine,
importModuleDynamically(specifier, _, importAssertions) {
const loader = asyncESM.esmLoader;
return loader.import(specifier, baseUrl, importAssertions);
return loader.dynamicImport(specifier, baseUrl, importAssertions);
}
}));
if (print) {
Expand Down
7 changes: 5 additions & 2 deletions lib/repl.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,11 @@ const {
ObjectKeys,
ObjectSetPrototypeOf,
Promise,
PromisePrototypeThen,
ReflectApply,
RegExp,
RegExpPrototypeExec,
SafeMap,
SafePromiseRace,
SafeSet,
SafeWeakSet,
Expand Down Expand Up @@ -455,12 +457,13 @@ function REPLServer(prompt,
// Remove all "await"s and attempt running the script
// in order to detect if error is truly non recoverable
const fallbackCode = SideEffectFreeRegExpPrototypeSymbolReplace(/\bawait\b/g, code, '');
const dynamicImportCache = new SafeMap();
try {
vm.createScript(fallbackCode, {
filename: file,
displayErrors: true,
importModuleDynamically: (specifier, _, importAssertions) => {
return asyncESM.esmLoader.import(specifier, parentURL,
return asyncESM.esmLoader.dynamicImport(specifier, parentURL,
importAssertions);
}
});
Expand Down Expand Up @@ -504,7 +507,7 @@ function REPLServer(prompt,
filename: file,
displayErrors: true,
importModuleDynamically: (specifier, _, importAssertions) => {
return asyncESM.esmLoader.import(specifier, parentURL,
return asyncESM.esmLoader.dynamicImport(specifier, parentURL,
importAssertions);
}
});
Expand Down
25 changes: 25 additions & 0 deletions test/es-module/test-esm-dynamic-import-mutating-fs.js
Original file line number Diff line number Diff line change
@@ -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('./target.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());
21 changes: 21 additions & 0 deletions test/es-module/test-esm-dynamic-import-mutating-fs.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import '../common/index.mjs';
import tmpdir from '../common/tmpdir.js';

import assert from 'node:assert';
import fs from 'node:fs/promises';
import { pathToFileURL } from 'node:url';

tmpdir.refresh();
const tmpDir = pathToFileURL(tmpdir.path);

const target = new URL('./target.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);

0 comments on commit 5569c58

Please sign in to comment.