Skip to content
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.

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

esm: add --experimental-import-non-javascript-without-assertion flag #40210

Closed

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 24, 2021

Restricts import of JSON modules to the assertion form only, unless the --experimental-import-non-javascript-without-assertion CLI flag is provided.

The first two commits are from #39921, which should land before this one.
This PR aims to help unblock #37375 by allowing unflagging only the assertion-required JSON imports first, leaving the unflagging of assertionless form for a later time.

Restricts import of JSON modules to the assertion form only, unless the
`--experimental-import-non-javascript-without-assertion` CLI flag is
provided.
@aduh95 aduh95 added experimental Issues and PRs related to experimental features. esm Issues and PRs related to the ECMAScript Modules implementation. labels Sep 24, 2021
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 24, 2021
@aduh95 aduh95 force-pushed the assertionless-json-import-behind-a-flag branch from 1fffc54 to 7da6afc Compare September 24, 2021 22:09
Comment on lines +1934 to +1935
An attempt was made to impor without an assertion a module that requires a
specific import assertion to be loaded.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
An attempt was made to impor without an assertion a module that requires a
specific import assertion to be loaded.
An attempt was made to import a module without an assertion that requires
a specific import assertion to be loaded.

@@ -54,6 +54,10 @@ executed in specific contexts.
<!-- YAML
added: v0.3.1
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/39921
description: Added suppoort of import assertions to the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description: Added suppoort of import assertions to the
description: Added support of import assertions to the

Typo.

changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/39921
description: Added suppoort of import assertions to the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description: Added suppoort of import assertions to the
description: Added support of import assertions to the

Typo.

@@ -852,6 +867,10 @@ const vm = require('vm');
<!-- YAML
added: v10.10.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/39921
description: Added suppoort of import assertions to the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description: Added suppoort of import assertions to the
description: Added support of import assertions to the

Typo.

@@ -1068,6 +1089,10 @@ vm.measureMemory({ mode: 'detailed', execution: 'eager' })
<!-- YAML
added: v0.3.1
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/39921
description: Added suppoort of import assertions to the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description: Added suppoort of import assertions to the
description: Added support of import assertions to the

Typo.

@@ -1145,6 +1172,10 @@ console.log(contextObject);
<!-- YAML
added: v0.3.1
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/39921
description: Added suppoort of import assertions to the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description: Added suppoort of import assertions to the
description: Added support of import assertions to the

Typo.

@@ -1247,6 +1280,10 @@ console.log(contextObject);
<!-- YAML
added: v0.3.1
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/39921
description: Added suppoort of import assertions to the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description: Added suppoort of import assertions to the
description: Added support of import assertions to the

Typo.

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

As I wrote in #39921 (comment), I don’t support adding the ability for an import statement to optionally have an assertion or not. Even though it’s behind a flag, this is complexity that loaders will need to support as soon as it’s in the Node codebase. Until it becomes clear that supporting both forms for the same file type is required by spec or use case, I don’t want to incur the complexity and maintenance burden of supporting this. This was also the path forward agreed at yesterday’s TSC meeting.

I would be happy to support a PR that adds an assertion syntax requirement for JSON modules, with nothing more.

@GeoffreyBooth GeoffreyBooth added loaders Issues and PRs related to ES module loaders loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team labels Sep 25, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Sep 25, 2021

I think we need to discuss at some point what are the technical challenges of supporting both syntaxes and (how) can they be overcome?.

If we find out that there are unsolvable technical road blocks, then this PR can be closed, the current experimental JSON support can be removed and a good chunk of #39921 can be re-written. It would also be certainly useful info for TC39 in case future proposal are raised to bring assertionless syntax to other formats.
If we find out that there are no technical blockers, then I think we should land #39921 (assuming it's technically good), then #40210, then #37375.

What I take out of the TSC meeting that we needed to ship/unflag first the syntax with assertions, and discuss unflagging the assertionless syntax later. I didn't understand that we meant removing the assertionless implementation completely – and I don't think we should unless there is a technical blocker (but I won't block a PR doing that to be clear).

Sure if you plan on having a alternative PR, I'm fine waiting for it to be opened before having the discussion, or sooner if it fits better the loaders agenda.

@jasnell
Copy link
Member

jasnell commented Sep 25, 2021

--Can't-say-that-I'm-all-that-in-love-with-the-long-cli-flag ;-)

Any chance we could shorten that up?

@GeoffreyBooth
Copy link
Member

That's not my recollection of the TSC meeting.

I also don't see the point in doing an analysis of the difficulty of supporting two syntaxes. It's obviously more complex than only supporting one per type, and that's enough. I don't find the use case compelling enough to justify supporting such complexity.

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 25, 2021

It’s not an analysis of the difficulty, but the feasibility: it’s clear that some folks want to have this feature, so this discussion need to happen at some point.

My thinking if it is possible to support both syntaxes, it would be counterproductive to remove it when the PR to make it work is already available, but feel free to disagree on that. If the conclusion of said discussion is: “it’s not it’s actually possible to support both for X reason”, it would help unblock the crispation around JSON module unflagging I think.

@GeoffreyBooth GeoffreyBooth removed the loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team label Oct 1, 2021
@GeoffreyBooth
Copy link
Member

Removing loaders-agenda from this one and moving it to #40250.

@aduh95 aduh95 marked this pull request as draft October 5, 2021 22:55
@GeoffreyBooth
Copy link
Member

Closing as #40250 is the version under active development.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features. lib / src Issues and PRs related to general changes in the lib or src directory. loaders Issues and PRs related to ES module loaders needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants