Skip to content

esm: add --import flag#43942

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
MoLow:add-experimental-import
Jul 31, 2022
Merged

esm: add --import flag#43942
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
MoLow:add-experimental-import

Conversation

@MoLow

@MoLow MoLow commented Jul 22, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes: #40110

todo:

  • add tests

@MoLow MoLow force-pushed the add-experimental-import branch from 06de59d to 198f88f Compare July 22, 2022 07:16
@nodejs-github-bot

nodejs-github-bot commented Jul 22, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Jul 22, 2022
@MoLow MoLow added the esm Issues and PRs related to the ECMAScript Modules implementation. label Jul 22, 2022
aduh95
aduh95 reviewed Jul 22, 2022
View reviewed changes
Comment thread doc/api/cli.md Outdated
Comment thread doc/api/cli.md Outdated
Comment thread lib/internal/process/esm_loader.js Outdated
@JakobJingleheimer

This comment was marked as outdated.

Sign in to view
@aduh95

aduh95 commented Jul 22, 2022

Copy link
Copy Markdown
Contributor

I thought this functionality is already possible with the globalPreload hook.

node/doc/api/esm.md

Lines 940 to 941 in d2fe72a

scope that the application runs in. This hook allows the return of a string
that is run as a sloppy-mode script on startup.

--import is about loading ESM, so it's definitely not the same thing as sloppy-mode script. See #40110 if you need context on this feature.

@MylesBorins

MylesBorins commented Jul 22, 2022

Copy link
Copy Markdown
Contributor

Is this still spec compliant?

@targos

targos commented Jul 22, 2022

Copy link
Copy Markdown
Member

@MylesBorins What part of the spec is relevant?

GeoffreyBooth
GeoffreyBooth reviewed Jul 22, 2022
View reviewed changes

@GeoffreyBooth GeoffreyBooth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This needs tests, including around variations of async behaviors:

  • An --import module that contains top-level await.
  • An --import module that does something async. Do we automatically await it before the next --import module runs? Before user code runs?

Comment thread src/node_options.cc Outdated
Comment thread src/node_options.cc Outdated
@guybedford

guybedford commented Jul 22, 2022

Copy link
Copy Markdown
Contributor

Overall I'm very much in favour of this, thanks for the PR!

I didn't follow the full thread, but what is the reasoning for starting with --experimental-import instead of just --import here? We can still label the feature as experimental without needing the experimental prefix IMO.

@MoLow MoLow force-pushed the add-experimental-import branch from 198f88f to 5a15e7a Compare July 23, 2022 21:50
JakobJingleheimer
JakobJingleheimer reviewed Jul 24, 2022
View reviewed changes
Comment thread lib/internal/process/esm_loader.js Outdated
Comment thread lib/internal/process/esm_loader.js Outdated
Comment thread lib/internal/process/esm_loader.js Outdated
Comment thread lib/internal/process/esm_loader.js
@MoLow MoLow force-pushed the add-experimental-import branch from dfad8f5 to a7d6bae Compare July 25, 2022 14:22
aduh95
aduh95 reviewed Jul 25, 2022
View reviewed changes
Comment thread doc/api/cli.md Outdated
@MoLow

MoLow commented Jul 25, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

added some tests, I am not sure if these are enough tests. please direct me to additional cases that need testing if I missed some.
in addition, I am ok with removing the --import from the flag name, just added it since it was what was written in the original issue
CC @GeoffreyBooth

aduh95
aduh95 reviewed Jul 25, 2022
View reviewed changes
Comment thread doc/api/cli.md Outdated
@MoLow MoLow changed the title esm: add --experimental-import flag esm: add --import flag Jul 25, 2022
@MoLow MoLow added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jul 25, 2022
GeoffreyBooth
GeoffreyBooth reviewed Jul 25, 2022
View reviewed changes
Comment thread doc/api/cli.md Outdated
Comment thread doc/api/cli.md Outdated
Comment thread doc/api/cli.md Outdated
Comment thread doc/api/cli.md Outdated
Comment thread test/es-module/test-esm-import-flag.js Outdated
Comment thread test/es-module/test-esm-import-flag.js Outdated
Comment thread test/fixtures/es-modules/print_imported.mjs Outdated
Comment thread lib/internal/process/esm_loader.js
aduh95
aduh95 reviewed Jul 25, 2022
View reviewed changes
Comment thread test/es-module/test-esm-import-flag.js Outdated
Comment thread test/es-module/test-esm-import-flag.js Outdated
Comment thread test/es-module/test-esm-import-flag.js Outdated
Comment thread test/fixtures/es-modules/esm-top-level-await.mjs Outdated
Comment thread test/fixtures/es-modules/esm-top-level-await.mjs Outdated
Comment thread test/fixtures/es-modules/print_imported.mjs Outdated
Comment thread test/fixtures/es-modules/esm-top-level-await.mjs Outdated
@MylesBorins

MylesBorins commented Jul 25, 2022

Copy link
Copy Markdown
Contributor

@MylesBorins What part of the spec is relevant?

I was curious about how this work in populating the internal cache and global state. Will this work similarly to subsequent script tags for example.

There might not be a spec issue, just wanted to confirm

@GeoffreyBooth

GeoffreyBooth commented Jul 25, 2022

Copy link
Copy Markdown
Member

Once this lands, I think we should mark it as blocked from release until we also land the “move loaders off thread” work, since that effort might effect --import. It would be similar to how we added the “backport-blocked-v18.x” tag to the chaining loaders PR until we had a few loaders-related PRs ready to release together. Thoughts? I know this will be experimental but I’m hopeful that the other PR isn’t too far behind and therefore we could release these together and minimize churn.

aduh95
aduh95 reviewed Jul 25, 2022
View reviewed changes
Comment thread test/es-module/test-esm-import-flag.js Outdated
Comment thread test/es-module/test-esm-loader-http-imports.mjs Outdated
Comment thread test/es-module/test-esm-loader-http-imports.mjs Outdated
Comment thread test/fixtures/es-modules/print_3.mjs Outdated
Comment thread test/fixtures/es-modules/esm-top-level-await.mjs Outdated
Comment thread test/fixtures/es-modules/esm-top-level-await.mjs Outdated
GeoffreyBooth
GeoffreyBooth reviewed Jul 26, 2022
View reviewed changes
Comment thread test/es-module/test-esm-import-flag.js Outdated
Comment thread test/fixtures/es-modules/print_3.mjs
Comment thread test/es-module/test-esm-loader-http-imports.mjs Outdated
Comment thread lib/internal/process/execution.js Outdated
@GeoffreyBooth GeoffreyBooth added module Issues and PRs related to the module subsystem. loaders Issues and PRs related to ES module loaders labels Jul 26, 2022
@MoLow MoLow requested a review from benjamingr July 26, 2022 16:04
This was referenced Nov 29, 2022
Closed
Closed
Closed
@frank-dspeed frank-dspeed mentioned this pull request Dec 4, 2022
Closed
@ruyadorno

ruyadorno commented Sep 7, 2023

Copy link
Copy Markdown
Member

This will need manual backport in case we want to land it in v18.

@GeoffreyBooth

GeoffreyBooth commented Sep 7, 2023

Copy link
Copy Markdown
Member

Yes, we need this on 18 to support the module customization hooks.

@aduh95 aduh95 mentioned this pull request Sep 7, 2023
Closed
@ruyadorno ruyadorno mentioned this pull request Sep 8, 2023
Merged
styfle
styfle reviewed Sep 19, 2023
View reviewed changes
Comment thread doc/api/cli.md
[#42511]: https://github.com/nodejs/node/issues/42511
[Chrome DevTools Protocol]: https://chromedevtools.github.io/devtools-protocol/
[CommonJS]: modules.md
[CommonJS module]: modules.md

@styfle styfle Sep 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like you have two links to the same page here

@daquinoaldo daquinoaldo mentioned this pull request Feb 28, 2025
Closed
@coderabbitai coderabbitai Bot mentioned this pull request Apr 13, 2026
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb left review comments

@targos targos targos left review comments

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@benjamingr benjamingr benjamingr approved these changes

@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

@aduh95 aduh95 aduh95 approved these changes

+1 more reviewer

@styfle styfle styfle left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders module Issues and PRs related to the module subsystem. process Issues and PRs related to the process subsystem. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add --import <module> flag for pre-loading ESM modules