Skip to content

module: unflag import assertions #39921

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

Closed
aduh95 wants to merge 13 commits into from
Closed

module: unflag import assertions #39921

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
8475f0e
vm: add support for import assertions in dynamic imports
aduh95 Sep 5, 2021
ece7a7d
module: unflag import assertions
aduh95 Aug 27, 2021
8754a41
fixup! module: unflag import assertions
aduh95 Sep 12, 2021
debc47f
fixup! module: unflag import assertions
aduh95 Sep 12, 2021
733deb8
fixup! fixup! module: unflag import assertions
aduh95 Sep 12, 2021
4fa6f58
fixup! module: unflag import assertions
aduh95 Sep 13, 2021
1e6330a
fixup! module: unflag import assertions
aduh95 Sep 13, 2021
1ecb35a
fixup! module: unflag import assertions
aduh95 Sep 13, 2021
180099e
fixup! module: unflag import assertions
aduh95 Sep 13, 2021
83ff00e
fixup! module: unflag import assertions
aduh95 Sep 13, 2021
b5ad8b0
fixup! module: unflag import assertions
aduh95 Sep 13, 2021
d364844
fixup! module: unflag import assertions
aduh95 Sep 14, 2021
d19ecc4
fixup! module: unflag import assertions
aduh95 Sep 21, 2021
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
fixup! module: unflag import assertions
  • Loading branch information
@aduh95
aduh95 committed Sep 13, 2021
commit 1e6330a0fe881cb77c80a7b2166f89945a7daed8
37 changes: 20 additions & 17 deletions lib/internal/modules/esm/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const { translators } = require(
'internal/modules/esm/translators');
const { getOptionValue } = require('internal/options');

const importAssertionTypeCache = new SafeWeakMap();
const finalFormatCache = new SafeWeakMap();
const supportedTypes = ObjectFreeze([undefined, 'json']);

Expand Down Expand Up @@ -231,26 +232,28 @@ class ESMLoader {
}
const { format, url } = await this.resolve(specifier, parentURL);

const jobMap = this.moduleMap.get(url);
let job;
let job = this.moduleMap.get(url);

if (jobMap != null) {
if (import_assertions.type != null && undefined in jobMap) {
job = jobMap[undefined];
// To avoid race conditions, always wait for non typed import to
// fulfill first.
if (job != null) {
const currentImportAssertionType = importAssertionTypeCache.get(job);
if (currentImportAssertionType === import_assertions.type) return job;

try {
// To avoid race conditions, always wait for previous module to fulfill first.
await job.modulePromise;
Comment thread
aduh95 marked this conversation as resolved.

const finalFormat = finalFormatCache.get(job);
if (import_assertions.type === 'json' && finalFormat !== 'json') {
throw new ERR_FAILED_IMPORT_ASSERTION(
url, 'type', import_assertions.type, finalFormat);
}
return job;
if (
import_assertions.type == null ||
(import_assertions.type === 'json' && finalFormat === 'json')
Copy link
Copy Markdown
Member

@bmeck bmeck Sep 23, 2021

Choose a reason for hiding this comment

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

shouldn't this always check if the finalFormat is 'json' and not skip it if it is nullish?

Copy link
Copy Markdown
Contributor Author

@aduh95 aduh95 Sep 23, 2021

Choose a reason for hiding this comment

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

If import_assertions.type is nullish (I.e. assertionless import) and the previous job succeeded, we return the cached module. Because there was no assertion made, there’s no reason to check what format the module was loaded with. If/when we want to force assertion for some format, we would need to add some additional logic, but that’s not what this PR aims to do.

shouldn't this always check if the finalFormat is 'json'

we still want non-JSON modules to load (such as module, commonjs, etc.), I’m not sure I understand what you mean by that.

Copy link
Copy Markdown
Contributor Author

@aduh95 aduh95 Sep 23, 2021

Choose a reason for hiding this comment

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

If import_assertions.type is nullish (I.e. assertionless import) and the previous job succeeded, we return the cached module. Because there was no assertion made, there’s no reason to check what format the module was loaded with. If/when we want to force assertion for some format, we would need to add some additional logic, but that’s not what this PR aims to do.

shouldn't this always check if the finalFormat is 'json'

we still want non-JSON modules to load (such as module, commonjs, etc.), I’m not sure I understand what you mean by that.

Copy link
Copy Markdown
Contributor Author

@aduh95 aduh95 Sep 23, 2021

Choose a reason for hiding this comment

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

Oh I think I get what you mean, indeed because json is the only valid value for an assertion type for now, indeed json is the only format acceptable here. I think we should keep the code ready for more values to be added, so when new assertion types are introduced, it won’t have to refactor the whole implementation.

Copy link
Copy Markdown
Member

@bmeck bmeck Sep 23, 2021

Choose a reason for hiding this comment

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

the problem is that we have 2 cache entries coalescing to the same module job and that makes me trying to figure out how a userland loader could recreate that behavior very complicated and I am definitely not confident in that collision being safe even with the mitigation I mentioned and requested above. currently we always have 1-1 fully resolved cache key to module instance. The concern is the many-1 model that we introduced by coalescing.

) return job;
return PromiseReject(new ERR_FAILED_IMPORT_ASSERTION(
url, 'type', import_assertions.type, finalFormat));
} catch {
// If the other job with a different type assertion failed, we got another chance.
job = undefined
}

job = jobMap[import_assertions.type];

// CommonJS will set functions for lazy job evaluation.
if (typeof job === 'function')
this.moduleMap.set(url, job = job(), import_assertions.type);
Expand All @@ -264,9 +267,8 @@ class ESMLoader {
if (import_assertions.type === 'json' && finalFormat !== 'json') {
throw new ERR_FAILED_IMPORT_ASSERTION(
url, 'type', import_assertions.type, finalFormat);
} else if (import_assertions.type == null) {
finalFormatCache.set(job, finalFormat);
}
finalFormatCache.set(job, finalFormat);

const translator = translators.get(finalFormat);

Expand All @@ -288,7 +290,8 @@ class ESMLoader {
inspectBrk
);

this.moduleMap.set(url, job, import_assertions.type);
importAssertionTypeCache.set(job, import_assertions.type);
this.moduleMap.set(url, job);

return job;
}
Expand Down
7 changes: 2 additions & 5 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 @@ -2,7 +2,6 @@

const ModuleJob = require('internal/modules/esm/module_job');
const {
ObjectCreate,
SafeMap,
} = primordials;
let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
Expand All @@ -18,16 +17,14 @@ class ModuleMap extends SafeMap {
validateString(url, 'url');
return super.get(url);
}
set(url, job, import_assertion_type) {
set(url, job) {
validateString(url, 'url');
if (job instanceof ModuleJob !== true &&
typeof job !== 'function') {
throw new ERR_INVALID_ARG_TYPE('job', 'ModuleJob', job);
}
debug(`Storing ${url} in ModuleMap`);
const jobMap = super.get(url) ?? ObjectCreate(null);
jobMap[import_assertion_type] = job;
return super.set(url, jobMap);
return super.set(url, job);
}
has(url) {
validateString(url, 'url');
Expand Down
5 changes: 5 additions & 0 deletions test/es-module/test-esm-dynamic-import-assertion.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ async function test() {
{ code: 'ERR_INVALID_IMPORT_ASSERTION' }
);

Comment thread
aduh95 marked this conversation as resolved.
await rejects(
import('specifier', { assert: { type: 'undefined' } }),
{ code: 'ERR_INVALID_IMPORT_ASSERTION' }
);

{
const [secret0, secret1] = await Promise.all([
import('../fixtures/experimental.json'),
Copy link
Copy Markdown
Member

@GeoffreyBooth GeoffreyBooth Sep 14, 2021

Choose a reason for hiding this comment

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

All these tests that import JSON without the assertion lack consensus. Please remove them (and associated documentation) if you’d like to land this PR.

Expand Down
5 changes: 5 additions & 0 deletions test/es-module/test-esm-dynamic-import-assertion.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ await rejects(
{ code: 'ERR_INVALID_IMPORT_ASSERTION' }
);

await rejects(
import('specifier', { assert: { type: 'undefined' } }),
{ code: 'ERR_INVALID_IMPORT_ASSERTION' }
);

{
const [secret0, secret1] = await Promise.all([
import('../fixtures/experimental.json'),
Expand Down