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

vm: add support for import assertions in dynamic imports #40249

Closed
wants to merge 2 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 28, 2021

No description provided.

@aduh95 aduh95 requested a review from devsnek September 28, 2021 20:41
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels Sep 28, 2021
doc/api/vm.md Outdated Show resolved Hide resolved
doc/api/vm.md Outdated Show resolved Hide resolved
@aduh95 aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 28, 2021
@aduh95 aduh95 requested a review from devsnek September 28, 2021 22:48
doc/api/vm.md Outdated Show resolved Hide resolved
doc/api/vm.md Outdated Show resolved Hide resolved
doc/api/vm.md Outdated Show resolved Hide resolved
doc/api/vm.md Show resolved Hide resolved
@aduh95
Copy link
Contributor Author

aduh95 commented Oct 1, 2021

Ping @nodejs/vm @nodejs/modules for reviews.

Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

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

Assert enforcement of spec for "type": "json" isn't in place but this is still experimental and I honestly don't have a clean way to enforce it even if we wanted to. We could treat this as a power/debugging tool which often lies outside of the spec? Seems fine for now but should revisit decision before unflagging.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 1, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 1, 2021
@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBooth
Copy link
Member

In #40250 (comment) I asked if this could wait until that PR lands, or if this could land as part of that PR. There’s an unresolved discussion there about when and whether import assertions should be passed around between internal functions, which is what this PR adds; so it seems premature to land this until we’ve settled that question.

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 2, 2021

@GeoffreyBooth I've opened this PR to separate the changes in vm from the changes required to add support to the ESM loader. My reasoning was:

  • It avoids the discussion being limited to discussing loader implementation and JSON modules in particular. As a proof, the discussion in this thread has been productive, folks have made great suggestions that indeed improves the PR.
  • No matter what happens with the other PRs, we need to fix at some point the support in vm (currently it supports assertions only for static imports, not dynamic ones, which was almost certainly an oversight of vm: add import assertion support #37176).
  • Landing this would also simplify reviewing the other PRs.
  • If none of my other PRs land, I'd like to at least land this so all the work I put into it is not lost.
  • Landing this doesn't mean that no other changes can be made relative to import assertions support in vm, the ES modules support there is still experimental and behind a flag (well two flags even: --experimental-vm-modules --harmony-import-assertions).

That being said, of course I'm OK waiting if there are concerns, but if there are none, I'd rather have it landed, and a future PR can change the implementation again if it turns out being necessary.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Seems like a nice simple change, and since VM doesn't relate to cache keying questions at all I don't see any reason not to land this.

Comment on lines 605 to 613
Local<Object> assertions =
Object::New(isolate, v8::Null(env->isolate()), nullptr, nullptr, 0);
for (int i = 0; i < import_assertions->Length(); i += 2) {
assertions
->Set(env->context(),
Local<String>::Cast(import_assertions->Get(env->context(), i)),
Local<Value>::Cast(import_assertions->Get(env->context(), i + 1)))
.ToChecked();
}
Copy link
Member

Choose a reason for hiding this comment

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

if possible it might be good to dedupe this code with the static import path so they stay consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in adf6993.

@aduh95 aduh95 force-pushed the vm-import-assertion-dynamic branch from 9c2964f to adf6993 Compare October 5, 2021 17:31
@aduh95 aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Oct 5, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2021
@nodejs-github-bot
Copy link
Collaborator

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.

I discussed with @aduh95; my concern about the function signature was whether the assertions would become part of the cache key. He tells me that he thinks that won’t be the case, in this PR at least. So I think this should be fine to land.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 9, 2021
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 9, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2021

Landed in 879ff77...08ffbd1

@github-actions github-actions bot closed this Oct 9, 2021
nodejs-github-bot pushed a commit that referenced this pull request Oct 9, 2021
PR-URL: #40249
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
@aduh95 aduh95 deleted the vm-import-assertion-dynamic branch October 9, 2021 08:59
targos pushed a commit that referenced this pull request Oct 13, 2021
PR-URL: #40249
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
richardlau added a commit that referenced this pull request Oct 18, 2021
Notable changes:

deps:
  * upgrade npm to 8.1.0 (npm team) #40463
doc:
  * deprecate (doc-only) http abort related (dr-js) #36670
vm:
  * (SEMVER-MINOR) add support for import assertions in dynamic imports (Antoine du Hamel) #40249

PR-URL: #40504
@richardlau richardlau mentioned this pull request Oct 18, 2021
richardlau added a commit that referenced this pull request Oct 19, 2021
Notable changes:

deps:
  * upgrade npm to 8.1.0 (npm team) #40463
doc:
  * deprecate (doc-only) http abort related (dr-js) #36670
vm:
  * (SEMVER-MINOR) add support for import assertions in dynamic imports (Antoine du Hamel) #40249

PR-URL: #40504
richardlau added a commit that referenced this pull request Oct 19, 2021
Notable changes:

deps:
  * upgrade npm to 8.1.0 (npm team) #40463
doc:
  * deprecate (doc-only) http abort related (dr-js) #36670
vm:
  * (SEMVER-MINOR) add support for import assertions in dynamic imports (Antoine du Hamel) #40249

PR-URL: #40504
richardlau added a commit that referenced this pull request Oct 19, 2021
Notable Changes:

Experimental ESM Loader Hooks API:

Node.js ESM Loader hooks have been consolidated to represent the steps involved needed to facilitate future loader chaining:
1. `resolve`: `resolve` [+ `getFormat`]
2. `load`: `getFormat` + `getSource` + `transformSource`

For consistency, `getGlobalPreloadCode` has been renamed to `globalPreload`.

A loader exporting obsolete hook(s) will trigger a single deprecation warning (per loader) listing the errant hooks.

Contributed by Jacob Smith, Geoffrey Booth, and Bradley Farias - #37468

Other Notable Changes:

deps:
  * upgrade npm to 8.1.0 (npm team) #40463
doc:
  * deprecate (doc-only) http abort related (dr-js) #36670
vm:
  * (SEMVER-MINOR) add support for import assertions in dynamic imports (Antoine du Hamel) #40249

PR-URL: #40504
richardlau added a commit that referenced this pull request Oct 20, 2021
Notable Changes:

Experimental ESM Loader Hooks API:

Node.js ESM Loader hooks have been consolidated to represent the steps involved needed to facilitate future loader chaining:
1. `resolve`: `resolve` [+ `getFormat`]
2. `load`: `getFormat` + `getSource` + `transformSource`

For consistency, `getGlobalPreloadCode` has been renamed to `globalPreload`.

A loader exporting obsolete hook(s) will trigger a single deprecation warning (per loader) listing the errant hooks.

Contributed by Jacob Smith, Geoffrey Booth, and Bradley Farias - #37468

Other Notable Changes:

deps:
  * upgrade npm to 8.1.0 (npm team) #40463
doc:
  * deprecate (doc-only) http abort related (dr-js) #36670
vm:
  * (SEMVER-MINOR) add support for import assertions in dynamic imports (Antoine du Hamel) #40249

PR-URL: #40504
@Ashoat
Copy link

Ashoat commented May 20, 2022

Running Node 16.13.2, I see the following when I try to specify an import assertion on a dynamic import():

    const importedJSON = await import(`../../${path}`, {
                                                     ^

SyntaxError: Unexpected token ','

I think Node 16.3.2 includes this commit, but I'm guessing there's another one that's necessary in order for this to work?

Thanks in advance for any response, and sorry for pinging the PR like this.

@aduh95
Copy link
Contributor Author

aduh95 commented May 20, 2022

You need to use the --harmony-import-assertions CLI flag, or even better update to Node.js v16.15+.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants