GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — esm: restore `next<HookName>`'s `context` as optional arg by JakobJingleheimer · Pull Request #43553 · nodejs/node

Conversation

@JakobJingleheimer

@JakobJingleheimer JakobJingleheimer commented Jun 23, 2022

Copy link
Copy Markdown
Member

This PR ports some internal code improvements otherwise lost when #43363 was reverted. It also contains a small bugfix wherein the context argument of next<HookName>() (or default<HookName>() prior to chaining) was actually optional.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and Kifzy reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@JakobJingleheimer JakobJingleheimer added esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders. labels Jun 23, 2022
@JakobJingleheimer
JakobJingleheimer requested review from GeoffreyBooth and aduh95 June 23, 2022 22:10
@nodejs-github-bot

nodejs-github-bot commented Jun 23, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/modules
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 23, 2022
GeoffreyBooth
GeoffreyBooth approved these changes Jun 23, 2022
View reviewed changes
aduh95
aduh95 reviewed Jun 23, 2022
View reviewed changes
Comment thread lib/internal/modules/esm/loader.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/modules/esm/loader.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Jun 24, 2022
View reviewed changes
Comment thread lib/internal/modules/esm/loader.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Jun 24, 2022
View reviewed changes
Comment thread test/fixtures/es-module-loaders/loader-load-null-return.mjs Outdated

@aduh95 aduh95 Jun 24, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can we also have a test for when the return value is null instead of Promise<null>?

Suggested change
export async function load(specifier, context, next) {
export function load(specifier, context, next) {

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@JakobJingleheimer JakobJingleheimer Jun 24, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yep! Will do tomorrow (alternatively in the tidy follow-up I'm about to do)

PS gaah, yah killin me with the drip-feeding 😜

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
aduh95
aduh95 approved these changes Jun 24, 2022
View reviewed changes

@aduh95 aduh95 left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM assuming #43558 fixes the failing test

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@JakobJingleheimer
JakobJingleheimer force-pushed the esm/fix-optional-context-arg branch from 4227327 to 1f380d1 Compare June 25, 2022 11:22
jasnell
jasnell approved these changes Jun 27, 2022
View reviewed changes
JakobJingleheimer added 3 commits July 1, 2022 20:33
@JakobJingleheimer
esm: restore next<HookName>'s context as optional arg
581d8a8
@JakobJingleheimer
fixup: add test case for null hook returns
d510852
@JakobJingleheimer
fixup: switch back to regular value arg to error
cb738d6
@JakobJingleheimer
JakobJingleheimer force-pushed the esm/fix-optional-context-arg branch from 1f380d1 to cb738d6 Compare July 1, 2022 18:35
@nodejs-github-bot

nodejs-github-bot commented Jul 1, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45037/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
GeoffreyBooth
GeoffreyBooth approved these changes Jul 1, 2022
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Jul 1, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45042/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jul 2, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45043/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jul 2, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/45044/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@GeoffreyBooth GeoffreyBooth added commit-queue-squash PRs the Commit Queue should land as one squashed commit. commit-queue PRs queued for automated landing through the Commit Queue. and removed needs-ci PRs that need a full CI run. labels Jul 2, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jul 2, 2022
@nodejs-github-bot
nodejs-github-bot merged commit 1f6d005 into nodejs:main Jul 2, 2022
@nodejs-github-bot

nodejs-github-bot commented Jul 2, 2022

Copy link
Copy Markdown
Collaborator

Landed in 1f6d005
JakobJingleheimer reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@GeoffreyBooth
GeoffreyBooth deleted the esm/fix-optional-context-arg branch July 2, 2022 04:53
@github-actions github-actions Bot mentioned this pull request Jul 4, 2022
CI Reliability 2022-07-04 nodejs/reliability#317
Open
23 tasks
@github-actions github-actions Bot mentioned this pull request Jul 8, 2022
CI Reliability 2022-07-08 nodejs/reliability#318
Open
16 tasks
targos pushed a commit that referenced this pull request Jul 12, 2022
@JakobJingleheimer @targos
esm: restore next<HookName>'s context as optional arg
953fefe 
PR-URL: #43553
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Jul 12, 2022
v18.6.0 release proposal #43789
Merged
targos pushed a commit that referenced this pull request Jul 20, 2022
@JakobJingleheimer @targos
esm: restore next<HookName>'s context as optional arg
1cda175 
PR-URL: #43553
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
@JakobJingleheimer @targos
esm: restore next<HookName>'s context as optional arg
33a2902 
PR-URL: #43553
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Aug 3, 2022
v16.17.0 release proposal #44098
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@JakobJingleheimer @guangwong
esm: restore next<HookName>'s context as optional arg
8430cf9 
PR-URL: nodejs/node#43553
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes
@aduh95 aduh95 aduh95 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

commit-queue-squash PRs the Commit Queue should land as one squashed commit. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@JakobJingleheimer @nodejs-github-bot @jasnell @GeoffreyBooth @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.