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

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

Merged
merged 1 commit into from
Jun 22, 2022
Merged

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

merged 1 commit into from
Jun 22, 2022

Conversation

JakobJingleheimer
Copy link
Contributor

Reverts #43363

Per nodejs/loaders#90

@nodejs/releasers @danielleadams We decided to revert the PR which was previously marked as blocking several other PRs; those other PRs remain unblocked. Apologies for the confusion.

@JakobJingleheimer JakobJingleheimer added esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders labels Jun 21, 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 21, 2022
@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Jun 21, 2022 •
edited
Loading

Request fast track. @nodejs/loaders

@GeoffreyBooth GeoffreyBooth added the fast-track PRs that do not need to wait for 48 hours to land. label Jun 21, 2022
@github-actions
Copy link
Contributor

Fast-track has been requested by @GeoffreyBooth. Please 👍 to approve.

guybedford
guybedford approved these changes Jun 21, 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.

As discussed in the loaders meeting, this takes the pressure off the design space for now, and hopefully with some progress on threaded loaders, syncification of the resolver can become possible that way. And this PR can always be added back if it comes to that.

aduh95
aduh95 approved these changes Jun 21, 2022
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

RSLGTM

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@GeoffreyBooth
Copy link
Member

cc @juanarbol, please make sure that this revert is processed before the next release (in other words, let’s make sure that #43363 gets reverted and therefore doesn’t go out in the next release).

nodejs/Release#737

@richardlau
Copy link
Member

cc @juanarbol, please make sure that this revert is processed before the next release (in other words, let’s make sure that #43363 gets reverted and therefore doesn’t go out in the next release).

nodejs/Release#737

If #43363 hasn't gone out in a release we can mark it with dont-land-on-* labels.

@github-actions github-actions bot mentioned this pull request Jun 22, 2022
Open
35 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@JakobJingleheimer JakobJingleheimer added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2022
@nodejs-github-bot nodejs-github-bot merged commit 3c04034 into main Jun 22, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 3c04034

@aduh95 aduh95 deleted the revert-43363-esm/synchronous-resolve-hook branch June 22, 2022 07:15
@richardlau richardlau added dont-land-on-v14.x dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. labels Jun 22, 2022
@GeoffreyBooth
Copy link
Member

@richardlau This is the revert; we want it to land in any branch where the original PR landed . . . ?

@richardlau
Copy link
Member

@GeoffreyBooth AFAIK #43363 hasn't landed on any of the staging branches.

This was referenced Jun 23, 2022
Open
Open
F3n67u pushed a commit to F3n67u/node that referenced this pull request Jun 24, 2022
@JakobJingleheimer @F3n67u
Revert "esm: convert resolve hook to synchronous"
33bd56d 
This reverts commit 90b634a.

PR-URL: nodejs#43526
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
This was referenced Jun 25, 2022
Open
Open
Open
@GeoffreyBooth GeoffreyBooth mentioned this pull request Jul 4, 2022
Merged
3 tasks
@RafaelGSS RafaelGSS mentioned this pull request Oct 14, 2022
Merged
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

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

@aduh95 aduh95 aduh95 approved these changes

@guybedford guybedford guybedford approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@bmeck bmeck Awaiting requested review from bmeck

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. fast-track PRs that do not need to wait for 48 hours to land. loaders Issues and PRs related to ES module loaders needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

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