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.

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

[v22.x backport] module: add findPackageJSON util #56494

Open
wants to merge 3 commits into
base: v22.x
Choose a base branch
from

Conversation

JakobJingleheimer
Copy link
Member

PR-URL: #55412
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Antoine du Hamel [email protected]

PR-URL: #55543
Reviewed-By: Jacob Smith [email protected]
Reviewed-By: Marco Ippolito [email protected]

PR-URL: #56382
Reviewed-By: Jordan Harband [email protected]
Reviewed-By: Geoffrey Booth [email protected]

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels Jan 6, 2025
@JakobJingleheimer
Copy link
Member Author

JakobJingleheimer commented Jan 12, 2025

Oh son of a…. Forgot that would break the PR.

JakobJingleheimer and others added 3 commits January 12, 2025 14:14
PR-URL: nodejs#55412
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#55543
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@JakobJingleheimer
Copy link
Member Author

@ruyadorno could this land soon before more conflicts arise?

> supersedes it, and a loader hook supersedes that).

> **Caveat**: This currently leverages only the built-in default resolver; if
> [`resolve` customization hooks][resolve hook] are registered, they will not affect the resolution.
Copy link
Member Author

Choose a reason for hiding this comment

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

[resolve hook] here does not exist in 22.x, but it did in main, so it's not in the cherry-pick. @nodejs/releasers should I do to fix it?

Copy link
Member

Choose a reason for hiding this comment

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

well, in these cases you might need to manually amend a commit to add / remove

Copy link
Member

@ruyadorno ruyadorno Jan 16, 2025

Choose a reason for hiding this comment

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

In this specific case it looks like the right way to go about it is to insert the missing resolve hook link as part of the same commit that introduces this patch. Assuming that resolve hooks are actually available on the v22.x release line of course.

Copy link
Member Author

Choose a reason for hiding this comment

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

How should I go about that technically: that would no-longer be a cherry-picked commit.

I assume I would amend the commit, which would create a fresh hash. But I want to be sure, because that's a quasi-destructive action.

Copy link
Member

@marco-ippolito marco-ippolito Jan 16, 2025

Choose a reason for hiding this comment

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

Its ok if its a different hash, it is a different commit infact. The info is in the metadata of the commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants