FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

module: add `findNearestPackageJSON` utility by JakobJingleheimer · Pull Request #55173 · nodejs/node · GitHub

/ node Public

module: add findNearestPackageJSON utility - #55173

Closed
JakobJingleheimer wants to merge 2 commits into
nodejs:mainfrom
JakobJingleheimer:feat/module-utils-findPJSON
ConversationCommits (2)ChecksFiles changed
Closed

module: add findNearestPackageJSON utility#55173
JakobJingleheimer wants to merge 2 commits into
nodejs:mainfrom
JakobJingleheimer:feat/module-utils-findPJSON

Conversation

JakobJingleheimer commented Sep 30, 2024

Copy link
Copy Markdown
Member

This draft is not ready for review. Opening it in its current state for visibility.

Split from #54992

nodejs-github-bot commented Sep 30, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. typings labels Sep 30, 2024
JakobJingleheimer force-pushed the feat/module-utils-findPJSON branch from 87d2b59 to 6ddf9ab Compare September 30, 2024 10:54

JakobJingleheimer commented Sep 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

@anonrig what is the difference between GetNearestParentPackageJSON and GetPackageScopeConfig?

JakobJingleheimer mentioned this pull request Sep 30, 2024
Closed
anonrig reviewed Sep 30, 2024
View reviewed changes
Comment thread lib/internal/modules/package_json_reader.js
/**
* Find the nearest package.json
* @param {URL['pathname']} origin Where to start searching.
* @returns {URL['pathname']} The fully resolved location of the package.json file.

anonrig Sep 30, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Question: Do we need to also make sure this returns a UNC path on Windows? UNC path (the result of toNamespacedPath() is required if the path is extremely long. windows weirdly throws an error if long path feature is not enabled)

anonrig Sep 30, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
* @returns {URL['pathname']} The fully resolved location of the package.json file.
* @returns {URL['pathname'] | undefined} The fully resolved location of the package.json file.

JakobJingleheimer Sep 30, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Regarding your question: i have no idea. It sounds like you answered it yourself though (and the answer is "yes").

Comment thread src/node_modules.cc
void BindingData::GetNearestParentPackageJSON(
const v8::FunctionCallbackInfo<v8::Value>& args) {
void BindingData::FindNearestParentPackageJSON(
const v8::FunctionCallbackInfo<v8::Value>& args

anonrig Sep 30, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This change seems to be unrelated

JakobJingleheimer Sep 30, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Technically yes, this un-mangling is not required to make the change work.

anonrig commented Sep 30, 2024

Copy link
Copy Markdown
Member

@anonrig what is the difference between GetNearestParentPackageJSON and c?

GetNearestParentPackageJSON:

  • Traverses through the root of the current file until it finds a package.json file and returns it.

GetNearestParentPackageJSON

  • Does the same thing but accepts a boolean flag to add slash.

JakobJingleheimer commented Sep 30, 2024

Copy link
Copy Markdown
Member Author

This was superfluous 😞

JakobJingleheimer mentioned this pull request Oct 16, 2024
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

anonrig anonrig left review comments

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. typings

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL