module: execute `--import` sequentially #50474

aduh95 · 2023-10-30T10:21:43Z

nodejs-github-bot · 2023-10-30T10:21:48Z

Review requested:

@nodejs/loaders

doc/api/cli.md

test/fixtures/es-modules/esm-top-level-await.mjs

Uzlopak · 2023-10-30T10:57:29Z

If one of the modules has a top level await you have to load the modules in order, but if not you could load them unordered.
So it is a fix which can have performance regressions?

JakobJingleheimer · 2023-10-30T14:05:36Z

So it is a fix which can have performance regressions?

I think it's more accurate to say this has performance implications.

anonrig

I'm -1 on this. Is it really worth preserving the order when sequential awaiting has a huge impact on performance?

aduh95 · 2023-10-30T15:31:45Z

has a huge impact on performance?

You say huge, how big are we talking about?

anonrig · 2023-10-30T15:36:33Z

has a huge impact on performance?

You say huge, how big are we talking about?

That's a good question that deserves a benchmark 👍

GeoffreyBooth · 2023-10-30T15:56:44Z

I’m -1 on this. Is it really worth preserving the order when sequential awaiting has a huge impact on performance?

Yes, because the current implementation doesn’t achieve the desired effect. If the first --import registers hooks to handle TypeScript, the second --import of a TypeScript file should work. It doesn’t matter how much slower it becomes; the current behavior is broken.

If the user wants parallelized loading, they can put a bunch of import statements into a file and --import that.

@GeoffreyBooth

Convinced by @GeoffreyBooth's comments

aduh95 · 2023-10-30T16:09:53Z

has a huge impact on performance?

You say huge, how big are we talking about?

That's a good question that deserves a benchmark 👍

I don't think it's that good of a question, it was mostly rhetorical. The change has literally no impact on folks who are not using --import, and the impact of folks using lots of --import will depend a lot on what's inside the imported modules. I don't think we can come up with a benchmark showing regressions unless it's very artificial (i.e. no real-world use case would be impacted), but happy to be proven wrong.

--import is an experimental API, performance consideration is not a strong enough reason to block that PR from landing IMO (unless you're saying it will slow down users who are not using it, but I don't think it's the case).

giltayar · 2023-10-31T04:47:26Z

I’m -1 on this. Is it really worth preserving the order when sequential awaiting has a huge impact on performance?

@GeoffreyBooth supplied one reason it has to be sequential. Another reason is the "loader registration use case" as outlined in #50427 (and which was how this bug was found), where if the user writes --import loader-a --import loader-b then they want to register the loaders to be registered in that specific order because loader chain order is important: some loaders have to be chained in just the right order.

test/es-module/test-esm-import-flag.mjs

Co-authored-by: Jacob Smith <[email protected]>

nodejs-github-bot · 2023-10-31T19:08:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/55363/

nodejs-github-bot · 2023-10-31T21:03:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/55366/

nodejs-github-bot · 2023-10-31T21:03:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/55367/

nodejs-github-bot · 2023-11-01T05:42:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/55377/

nodejs-github-bot · 2023-11-01T10:30:55Z

Commit Queue failed

- Loading data for nodejs/node/pull/50474
✔  Done loading data for nodejs/node/pull/50474
----------------------------------- PR info ------------------------------------
Title      module: execute `--import` sequentially (#50474)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     aduh95:--import-serial -> nodejs:main
Labels     process, author ready, needs-ci, commit-queue-squash
Commits    2
 - module: execute `--import` sequentially
 - Apply suggestions from code review
Committers 2
 - Antoine du Hamel 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/50474
Fixes: https://github.com/nodejs/node/issues/50427
Reviewed-By: Jacob Smith 
Reviewed-By: Michaël Zasso 
Reviewed-By: Moshe Atlow 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50474
Fixes: https://github.com/nodejs/node/issues/50427
Reviewed-By: Jacob Smith 
Reviewed-By: Michaël Zasso 
Reviewed-By: Moshe Atlow 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - Apply suggestions from code review
   ℹ  This PR was created on Mon, 30 Oct 2023 10:21:43 GMT
   ✔  Approvals: 3
   ✔  - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/50474#pullrequestreview-1703780454
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/50474#pullrequestreview-1703829531
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/50474#pullrequestreview-1705908328
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-11-01T05:42:53Z: https://ci.nodejs.org/job/node-test-pull-request/55377/
- Querying data for job/node-test-pull-request/55377/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/6718540805

nodejs-github-bot · 2023-11-01T12:38:03Z

Landed in a3e09b3

PR-URL: nodejs#50474 Fixes: nodejs#50427 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

PR-URL: #50474 Fixes: #50427 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

nodejs/node#50474 로 수정된 nodejs `--import` 실행 순서 버그를 코드베이스에 반영함

module: execute --import sequentially

411f61c

nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Oct 30, 2023

JakobJingleheimer approved these changes Oct 30, 2023

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

test/fixtures/es-modules/esm-top-level-await.mjs Show resolved Hide resolved

targos approved these changes Oct 30, 2023

View reviewed changes

anonrig previously requested changes Oct 30, 2023

View reviewed changes

anonrig added the needs-benchmark-ci PR that need a benchmark CI run. label Oct 30, 2023

GeoffreyBooth removed the needs-benchmark-ci PR that need a benchmark CI run. label Oct 30, 2023

MoLow approved these changes Oct 31, 2023

View reviewed changes

arcanis reviewed Oct 31, 2023

View reviewed changes

test/es-module/test-esm-import-flag.mjs Show resolved Hide resolved

Apply suggestions from code review

174c719

Co-authored-by: Jacob Smith <[email protected]>

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2023

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 31, 2023

github-actions bot mentioned this pull request Nov 1, 2023

CI Reliability 2023-11-01 nodejs/reliability#704

Open

24 tasks

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 1, 2023

targos approved these changes Nov 1, 2023

View reviewed changes

aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 1, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 1, 2023

nodejs-github-bot merged commit a3e09b3 into nodejs:main Nov 1, 2023
37 checks passed

aduh95 deleted the --import-serial branch November 1, 2023 12:50

targos pushed a commit that referenced this pull request Nov 11, 2023

module: execute --import sequentially

cc6658d

PR-URL: #50474 Fixes: #50427 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

targos mentioned this pull request Nov 12, 2023

v21.2.0 release proposal #50681

Merged

targos pushed a commit that referenced this pull request Nov 14, 2023

module: execute --import sequentially

3606a0a

PR-URL: #50474 Fixes: #50427 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023

module: execute --import sequentially

77e8361

PR-URL: #50474 Fixes: #50427 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

UlisesGascon mentioned this pull request Dec 12, 2023

v20.11.0 proposal #51124

Merged

devunt mentioned this pull request Jan 26, 2024

node v20.11에서 수정된 import hook 실행 순서 버그를 반영함 penxle/withglyph#1128

Merged

robin-maki pushed a commit to penxle/withglyph that referenced this pull request Jan 26, 2024

node v20.11에서 수정된 import hook 실행 순서 버그를 반영함 (#1128)

01f8d8c

nodejs/node#50474 로 수정된 nodejs `--import` 실행 순서 버그를 코드베이스에 반영함

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: execute `--import` sequentially #50474

module: execute `--import` sequentially #50474

aduh95 commented Oct 30, 2023

nodejs-github-bot commented Oct 30, 2023

Uzlopak commented Oct 30, 2023

JakobJingleheimer commented Oct 30, 2023

anonrig left a comment

aduh95 commented Oct 30, 2023

anonrig commented Oct 30, 2023

GeoffreyBooth commented Oct 30, 2023

aduh95 commented Oct 30, 2023 •

edited

Loading

giltayar commented Oct 31, 2023 •

edited

Loading

nodejs-github-bot commented Oct 31, 2023

nodejs-github-bot commented Oct 31, 2023

nodejs-github-bot commented Oct 31, 2023

nodejs-github-bot commented Nov 1, 2023

nodejs-github-bot commented Nov 1, 2023

nodejs-github-bot commented Nov 1, 2023

module: execute --import sequentially #50474

module: execute --import sequentially #50474

Conversation

aduh95 commented Oct 30, 2023

nodejs-github-bot commented Oct 30, 2023

Uzlopak commented Oct 30, 2023

JakobJingleheimer commented Oct 30, 2023

anonrig left a comment

Choose a reason for hiding this comment

aduh95 commented Oct 30, 2023

anonrig commented Oct 30, 2023

GeoffreyBooth commented Oct 30, 2023

aduh95 commented Oct 30, 2023 • edited Loading

giltayar commented Oct 31, 2023 • edited Loading

nodejs-github-bot commented Oct 31, 2023

nodejs-github-bot commented Oct 31, 2023

nodejs-github-bot commented Oct 31, 2023

nodejs-github-bot commented Nov 1, 2023

nodejs-github-bot commented Nov 1, 2023

nodejs-github-bot commented Nov 1, 2023

module: execute `--import` sequentially #50474

module: execute `--import` sequentially #50474

aduh95 commented Oct 30, 2023 •

edited

Loading

giltayar commented Oct 31, 2023 •

edited

Loading