esm: convert `resolve` hook to synchronous #43363

JakobJingleheimer · 2022-06-09T18:15:44Z

Notable changes

This PR converts the resolve hook from asynchronous to synchronous to align with the general ecosystem (most namely browsers). When a resolve hook returns a thenable (eg async or promise), an error is specifically thrown.

It also converts import.meta.resolve() to synchronous.

Unblocks

nodejs-github-bot · 2022-06-09T18:15:48Z

Review requested:

@nodejs/modules

lib/internal/modules/esm/resolve.js

lib/internal/modules/esm/loader.js

test/es-module/test-esm-import-meta-resolve.mjs

test/es-module/test-esm-loader-search.js

Refs: nodejs#43363 (comment)

guybedford

Would love to see this!

lib/internal/modules/esm/initialize_import_meta.js

lib/internal/modules/esm/loader.js

test/es-module/test-esm-import-meta-resolve.mjs

nodejs-github-bot · 2022-06-18T16:22:09Z

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

nodejs-github-bot · 2022-06-18T17:22:59Z

Landed in 90b634a

Refs: #43363 (comment) PR-URL: #43374 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>

Notable Changes: cli: * (SEMVER-MINOR) add `--watch` (Moshe Atlow) #44366 esm: * remove specifier resolution flag (Geoffrey Booth) #44859 * convert `resolve` hook to synchronous (Jacob Smith) #43363 http: * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing) #44180 * (SEMVER-MAJOR) use Keep-Alive by default in global agents (Paolo Insogna) #43522 build: * (SEMVER-MAJOR) remove dtrace & etw support (Ben Noordhuis) #43652 * (SEMVER-MAJOR) remove systemtap support (Ben Noordhuis) #43651 Deprecation and Removals: * (SEMVER-MAJOR) runtime deprecate exports double slash maps (Guy Bedford) #44495 Semver-Major Commits: * [aa3a572] - (SEMVER-MAJOR) build: remove dtrace & etw support (Ben Noordhuis) #43652 * [38f1e27] - (SEMVER-MAJOR) build: remove systemtap support (Ben Noordhuis) #43651 * [2849283] - (SEMVER-MAJOR) crypto: remove non-standard `webcrypto.Crypto.prototype.CryptoKey` (Antoine du Hamel) #42083 * [a1653ac] - (SEMVER-MAJOR) crypto: do not allow to call setFips from the worker thread (Sergey Petushkov) #43624 * [a4fa526] - (SEMVER-MAJOR) fs: add directory autodetection to fsPromises.symlink() (Livia Medeiros) #42894 * [bb4891d] - (SEMVER-MAJOR) fs: add validateBuffer to improve error (Hirotaka Tagawa / wafuwafu13) #44769 * [950a441] - (SEMVER-MAJOR) fs: remove coercion to string in writing methods (Livia Medeiros) #42796 * [41a6d82] - (SEMVER-MAJOR) fs: harden fs.readSync(buffer, options) typecheck (LiviaMedeiros) #42772 * [2275faa] - (SEMVER-MAJOR) fs: harden fs.read(params, callback) typecheck (LiviaMedeiros) #42772 * [29953a0] - (SEMVER-MAJOR) fs: harden filehandle.read(params) typecheck (LiviaMedeiros) #42772 * [4267b92] - (SEMVER-MAJOR) http: use Keep-Alive by default in global agents (Paolo Insogna) #43522 * [f529f73] - (SEMVER-MAJOR) lib: brand check event handler property receivers (Chengzhong Wu) #44483 * [6de2673] - (SEMVER-MAJOR) lib: enable global WebCrypto by default (Antoine du Hamel) #42083 * [73ba883] - (SEMVER-MAJOR) lib: use private field in AbortController (Joyee Cheung) #43820 * [7dd2f41] - (SEMVER-MAJOR) module: runtime deprecate exports double slash maps (Guy Bedford) #44495 * [587367d] - (SEMVER-MAJOR) perf_hooks: expose webperf global scope interfaces (Chengzhong Wu) #44483 * [364c0e1] - (SEMVER-MAJOR) perf_hooks: fix webperf idlharness (Chengzhong Wu) #44483 * [e0ab8dd] - (SEMVER-MAJOR) process: make process.config read only (Sergey Petushkov) #43627 * [481a959] - (SEMVER-MAJOR) readline: remove `question` method from `InterfaceConstructor` (Antoine du Hamel) #44606 * [77e5856] - (SEMVER-MAJOR) src: turn embedder api overload into default argument (Alena Khineika) #43629 * [dabda03] - (SEMVER-MAJOR) src: per-environment time origin value (Chengzhong Wu) #43781 * [2b32985] - (SEMVER-MAJOR) stream: use null for the error argument (Luigi Pinca) #44312 * [57ff476] - (SEMVER-MAJOR) test: remove duplicate test (Luigi Pinca) #44051 * [77def91] - (SEMVER-MAJOR) tls,http2: send fatal alert on ALPN mismatch (Tobias Nießen) #44031 PR-URL: #44626 Co-authored-by: Ruy Adorno <ruyadorno@google.com>

Refs: nodejs/node#43363 (comment) PR-URL: nodejs/node#43374 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>

Notable Changes: cli: * (SEMVER-MINOR) add `--watch` (Moshe Atlow) #44366 esm: * remove specifier resolution flag (Geoffrey Booth) #44859 * convert `resolve` hook to synchronous (Jacob Smith) #43363 http: * (SEMVER-MINOR) add writeEarlyHints function to ServerResponse (Wing) #44180 * (SEMVER-MAJOR) use Keep-Alive by default in global agents (Paolo Insogna) #43522 build: * (SEMVER-MAJOR) remove dtrace & etw support (Ben Noordhuis) #43652 * (SEMVER-MAJOR) remove systemtap support (Ben Noordhuis) #43651 Deprecation and Removals: * (SEMVER-MAJOR) runtime deprecate exports double slash maps (Guy Bedford) #44495 Semver-Major Commits: * [aa3a572] - (SEMVER-MAJOR) build: remove dtrace & etw support (Ben Noordhuis) #43652 * [38f1e27] - (SEMVER-MAJOR) build: remove systemtap support (Ben Noordhuis) #43651 * [2849283] - (SEMVER-MAJOR) crypto: remove non-standard `webcrypto.Crypto.prototype.CryptoKey` (Antoine du Hamel) #42083 * [a1653ac] - (SEMVER-MAJOR) crypto: do not allow to call setFips from the worker thread (Sergey Petushkov) #43624 * [a4fa526] - (SEMVER-MAJOR) fs: add directory autodetection to fsPromises.symlink() (Livia Medeiros) #42894 * [bb4891d] - (SEMVER-MAJOR) fs: add validateBuffer to improve error (Hirotaka Tagawa / wafuwafu13) #44769 * [950a441] - (SEMVER-MAJOR) fs: remove coercion to string in writing methods (Livia Medeiros) #42796 * [41a6d82] - (SEMVER-MAJOR) fs: harden fs.readSync(buffer, options) typecheck (LiviaMedeiros) #42772 * [2275faa] - (SEMVER-MAJOR) fs: harden fs.read(params, callback) typecheck (LiviaMedeiros) #42772 * [29953a0] - (SEMVER-MAJOR) fs: harden filehandle.read(params) typecheck (LiviaMedeiros) #42772 * [4267b92] - (SEMVER-MAJOR) http: use Keep-Alive by default in global agents (Paolo Insogna) #43522 * [f529f73] - (SEMVER-MAJOR) lib: brand check event handler property receivers (Chengzhong Wu) #44483 * [6de2673] - (SEMVER-MAJOR) lib: enable global WebCrypto by default (Antoine du Hamel) #42083 * [73ba883] - (SEMVER-MAJOR) lib: use private field in AbortController (Joyee Cheung) #43820 * [7dd2f41] - (SEMVER-MAJOR) module: runtime deprecate exports double slash maps (Guy Bedford) #44495 * [587367d] - (SEMVER-MAJOR) perf_hooks: expose webperf global scope interfaces (Chengzhong Wu) #44483 * [364c0e1] - (SEMVER-MAJOR) perf_hooks: fix webperf idlharness (Chengzhong Wu) #44483 * [e0ab8dd] - (SEMVER-MAJOR) process: make process.config read only (Sergey Petushkov) #43627 * [481a959] - (SEMVER-MAJOR) readline: remove `question` method from `InterfaceConstructor` (Antoine du Hamel) #44606 * [77e5856] - (SEMVER-MAJOR) src: turn embedder api overload into default argument (Alena Khineika) #43629 * [dabda03] - (SEMVER-MAJOR) src: per-environment time origin value (Chengzhong Wu) #43781 * [2b32985] - (SEMVER-MAJOR) stream: use null for the error argument (Luigi Pinca) #44312 * [57ff476] - (SEMVER-MAJOR) test: remove duplicate test (Luigi Pinca) #44051 * [77def91] - (SEMVER-MAJOR) tls,http2: send fatal alert on ALPN mismatch (Tobias Nießen) #44031 PR-URL: #44626 Co-authored-by: Ruy Adorno <ruyadorno@google.com>

esm: convert resolve hook to synchronous

37abe52

JakobJingleheimer added esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders labels Jun 9, 2022

JakobJingleheimer added this to In progress in Loaders via automation Jun 9, 2022

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 9, 2022

GeoffreyBooth reviewed Jun 9, 2022

View changes

lib/internal/modules/esm/resolve.js Show resolved Hide resolved

aduh95 reviewed Jun 10, 2022

View changes

lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved

aduh95 reviewed Jun 10, 2022

View changes

lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved

aduh95 reviewed Jun 10, 2022

View changes

lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved

aduh95 reviewed Jun 10, 2022

View changes

test/es-module/test-esm-import-meta-resolve.mjs Outdated Show resolved Hide resolved

aduh95 reviewed Jun 10, 2022

View changes

test/es-module/test-esm-loader-search.js Outdated Show resolved Hide resolved

aduh95 added a commit to aduh95/node that referenced this pull request Jun 11, 2022

test: improve coverage for load hooks

c7be4b7

Refs: nodejs#43363 (comment)

aduh95 added a commit to aduh95/node that referenced this pull request Jun 11, 2022

test: improve coverage for load hooks

d3789b5

Refs: nodejs#43363 (comment)

aduh95 mentioned this pull request Jun 11, 2022

test: improve coverage for load hooks #43374

Merged

guybedford reviewed Jun 11, 2022

View changes

lib/internal/modules/esm/initialize_import_meta.js Show resolved Hide resolved

aduh95 reviewed Jun 12, 2022

View changes

lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved

aduh95 reviewed Jun 12, 2022

View changes

lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved

aduh95 reviewed Jun 12, 2022

View changes

test/es-module/test-esm-import-meta-resolve.mjs Outdated Show resolved Hide resolved

JakobJingleheimer added 3 commits Jun 12, 2022

fixup: fix import.meta.resolve

edb6ba5

fixup: correct assert.throws()

ae8ef63

fixup: support detecting when a resolve hook returns a promise

8bf7965

JakobJingleheimer force-pushed the esm/synchronous-resolve-hook branch 2 times, most recently from 8425665 to c3b979c Compare Jun 12, 2022

fixup: change async/thenable detection

ab935b6

JakobJingleheimer force-pushed the esm/synchronous-resolve-hook branch from c3b979c to ab935b6 Compare Jun 12, 2022

JakobJingleheimer added 2 commits Jun 12, 2022

fixup: update ESM doc for resolve hook

28d813f

fixup: revert back to arg destructuring

b332b97

This comment has been hidden.

Sign in to view

GeoffreyBooth mentioned this pull request Jun 14, 2022

esm: Make import.meta.resolve(…) take an options argument #31861

Closed

aduh95 approved these changes Jun 18, 2022

View changes

JakobJingleheimer added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 18, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 18, 2022

nodejs-github-bot merged commit 90b634a into nodejs:main Jun 18, 2022
60 checks passed

Loaders automation moved this from In progress to Done Jun 18, 2022

GeoffreyBooth deleted the esm/synchronous-resolve-hook branch Jun 18, 2022

GeoffreyBooth removed needs-ci PRs that need a full CI run. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jun 18, 2022

github-actions bot mentioned this pull request Jun 19, 2022

CI Reliability 2022-06-19 nodejs/reliability#305

Open

30 tasks

This was referenced Jun 19, 2022

Node.js Loaders Team Meeting 2022-06-21 nodejs/loaders#88

Closed

proposal: preImport nodejs/loaders#89

Closed

This was referenced Jun 20, 2022

CI Reliability 2022-06-20 nodejs/reliability#306

Open

CI Reliability 2022-06-21 nodejs/reliability#307

Open

JakobJingleheimer mentioned this pull request Jun 21, 2022

Revert "esm: convert resolve hook to synchronous" #43526

Merged

GeoffreyBooth added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 22, 2022

richardlau added dont-land-on-v14.x PRs that should not land on the v14.x-staging branch and should not be released in v14.x. dont-land-on-v16.x PRs that should not land on the v16.x-staging branch and should not be released in v16.x. labels Jun 22, 2022

JakobJingleheimer mentioned this pull request Jun 23, 2022

esm: restore next<HookName>'s context as optional arg #43553

Merged

RafaelGSS mentioned this pull request Oct 4, 2022

2022-10-18, Version 19.0.0 (Current) #44626

Merged

esm: convert `resolve` hook to synchronous #43363

esm: convert `resolve` hook to synchronous #43363

JakobJingleheimer commented Jun 9, 2022 •

edited

nodejs-github-bot commented Jun 9, 2022

guybedford left a comment

This comment has been hidden.

nodejs-github-bot commented Jun 18, 2022

nodejs-github-bot commented Jun 18, 2022

esm: convert resolve hook to synchronous #43363

esm: convert resolve hook to synchronous #43363

Conversation

JakobJingleheimer commented Jun 9, 2022 • edited

Notable changes

Unblocks

nodejs-github-bot commented Jun 9, 2022

guybedford left a comment

This comment has been hidden.

nodejs-github-bot commented Jun 18, 2022

nodejs-github-bot commented Jun 18, 2022

esm: convert `resolve` hook to synchronous #43363

esm: convert `resolve` hook to synchronous #43363

JakobJingleheimer commented Jun 9, 2022 •

edited