Skip to content

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

Merged
nodejs-github-bot merged 1 commit into
mainfrom
revert-43363-esm/synchronous-resolve-hook
Jun 22, 2022
Merged

Revert "esm: convert resolve hook to synchronous"#43526
nodejs-github-bot merged 1 commit into
mainfrom
revert-43363-esm/synchronous-resolve-hook

Conversation

@JakobJingleheimer

@JakobJingleheimerJakobJingleheimer commented Jun 21, 2022

Copy link
Copy Markdown
Member

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.

@JakobJingleheimerJakobJingleheimer 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

nodejs-github-bot commented Jun 21, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/modules

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

GeoffreyBooth commented Jun 21, 2022•
edited
Loading

Copy link
Copy Markdown
Member

Request fast track. @nodejs/loaders

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

github-actionsBot commented Jun 21, 2022

Copy link
Copy Markdown
Contributor

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

guybedford
guybedford approved these changes Jun 21, 2022
View reviewed changes

@guybedfordguybedford left a comment

Copy link
Copy Markdown
Contributor

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

@aduh95aduh95 left a comment

Copy link
Copy Markdown
Contributor

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

GeoffreyBooth commented Jun 21, 2022

Copy link
Copy Markdown
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

richardlau commented Jun 22, 2022

Copy link
Copy Markdown
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-actionsgithub-actionsBot mentioned this pull request Jun 22, 2022
Open
35 tasks
@nodejs-github-bot

nodejs-github-bot commented Jun 22, 2022

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Jun 22, 2022

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Jun 22, 2022

Copy link
Copy Markdown
Collaborator

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

@JakobJingleheimerJakobJingleheimer added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2022
@nodejs-github-botnodejs-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 mainJun 22, 2022
@nodejs-github-bot

nodejs-github-bot commented Jun 22, 2022

Copy link
Copy Markdown
Collaborator

Landed in 3c04034

@aduh95
aduh95 deleted the revert-43363-esm/synchronous-resolve-hook branch June 22, 2022 07:15
@GeoffreyBooth

GeoffreyBooth commented Jun 22, 2022

Copy link
Copy Markdown
Member

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

@richardlau

richardlau commented Jun 22, 2022

Copy link
Copy Markdown
Member

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

This was referenced Jun 23, 2022
Open
Open
This was referenced Jun 25, 2022
Open
Open
Open
@GeoffreyBoothGeoffreyBooth mentioned this pull request Jul 4, 2022
Merged
3 tasks
@RafaelGSSRafaelGSS mentioned this pull request Oct 14, 2022
Merged
This was referenced Jan 25, 2024
Merged
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GeoffreyBoothGeoffreyBoothGeoffreyBooth approved these changes
@guybedfordguybedfordguybedford approved these changes
@aduh95aduh95aduh95 approved these changes
@bmeckbmeckAwaiting requested review from bmeck
+1 more reviewer
@VoltrexKeyvaVoltrexKeyvaVoltrexKeyva approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

esmIssues and PRs related to the ECMAScript Modules implementation.fast-trackPRs that do not need to wait for 48 hours to land.loadersIssues and PRs related to ES module loadersneeds-ciPRs 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