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.

Sign up for GitHub

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

Jump to bottom

esm: convert resolve hook to synchronous #43363

Merged
merged 15 commits into from
Jun 18, 2022
Merged

esm: convert resolve hook to synchronous #43363

merged 15 commits into from
Jun 18, 2022

Conversation

JakobJingleheimer
Copy link
Member

@JakobJingleheimer JakobJingleheimer commented Jun 9, 2022 •
edited
Loading

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

@JakobJingleheimer 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
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 9, 2022
aduh95 added a commit to aduh95/node that referenced this pull request Jun 11, 2022
@aduh95
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
@aduh95
test: improve coverage for load hooks
d3789b5 
Refs: nodejs#43363 (comment)
@aduh95 aduh95 mentioned this pull request Jun 11, 2022
Merged
guybedford
guybedford reviewed Jun 11, 2022
View reviewed changes
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.

Would love to see this!

lib/internal/modules/esm/initialize_import_meta.js Show resolved Hide resolved
@JakobJingleheimer JakobJingleheimer force-pushed the esm/synchronous-resolve-hook branch 2 times, most recently from 8425665 to c3b979c Compare June 12, 2022 18:24
@JakobJingleheimer JakobJingleheimer force-pushed the esm/synchronous-resolve-hook branch from c3b979c to ab935b6 Compare June 12, 2022 18:38
@JakobJingleheimer

This comment was marked as resolved.

Sign in to view
@GeoffreyBooth GeoffreyBooth mentioned this pull request Jun 14, 2022
Closed
@nodejs-github-bot 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 nodejs-github-bot merged commit 90b634a into nodejs:main Jun 18, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 90b634a

@GeoffreyBooth GeoffreyBooth deleted the esm/synchronous-resolve-hook branch June 18, 2022 17:51
@GeoffreyBooth 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 github-actions bot mentioned this pull request Jun 19, 2022
Open
30 tasks
This was referenced Jun 19, 2022
Closed
Closed
This was referenced Jun 20, 2022
Open
Open
@JakobJingleheimer JakobJingleheimer mentioned this pull request Jun 21, 2022
Merged
@GeoffreyBooth 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
@JakobJingleheimer JakobJingleheimer mentioned this pull request Jun 23, 2022
Merged
targos pushed a commit that referenced this pull request Jul 12, 2022
@aduh95 @targos
test: improve coverage for load hooks
48ae00c 
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>
targos pushed a commit that referenced this pull request Jul 19, 2022
@aduh95 @targos
test: improve coverage for load hooks
24dfd70 
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>
targos pushed a commit that referenced this pull request Jul 31, 2022
@aduh95 @targos
test: improve coverage for load hooks
222a6e9 
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>
@RafaelGSS RafaelGSS mentioned this pull request Oct 4, 2022
Merged
RafaelGSS added a commit that referenced this pull request Oct 4, 2022
@RafaelGSS @ruyadorno
2022-10-18, Version 19.0.0 (Current)
ab691a5 
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>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@aduh95 @guangwong
test: improve coverage for load hooks
02f9e05 
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>
RafaelGSS added a commit that referenced this pull request Oct 11, 2022
@RafaelGSS @ruyadorno
2022-10-18, Version 19.0.0 (Current)
f85f907 
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>
RafaelGSS added a commit that referenced this pull request Oct 14, 2022
@RafaelGSS @ruyadorno
2022-10-18, Version 19.0.0 (Current)
a51472b 
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>
@ghost ghost mentioned this pull request Jul 10, 2023
Closed
This was referenced Jan 25, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guybedford guybedford guybedford left review comments

@jasnell jasnell jasnell left review comments

@ljharb ljharb ljharb left review comments

@aduh95 aduh95 aduh95 approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

Assignees
No one assigned
Labels
dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders notable-change PRs with changes that should be highlighted in changelogs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@JakobJingleheimer @nodejs-github-bot @jasnell @GeoffreyBooth @ljharb @guybedford @aduh95 @richardlau