Skip to content

module: expose getPackageJSON utility #55229

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

Closed
JakobJingleheimer wants to merge 37 commits into from
Closed

module: expose getPackageJSON utility #55229

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
48ab696
module: expose `getPackageJSON` utility
JakobJingleheimer Sep 30, 2024
dcddd1e
fixup: convert `path_value` to string before fn casting to path
JakobJingleheimer Oct 1, 2024
98c72f4
fixup: remove obsolete reference to `findNearestPackageJSON`
JakobJingleheimer Oct 1, 2024
d4ede54
fixup: add some tests cases
JakobJingleheimer Oct 1, 2024
92f2c62
feat: adjust `getNearestParentPackageJSON().path` to pjsonPath
JakobJingleheimer Oct 1, 2024
b83707b
feat: conditionally include non-required fields only when they exist
JakobJingleheimer Oct 1, 2024
e0beefa
fixup: remove unnecessary `JSON.parse()` (it's already parsed)
JakobJingleheimer Oct 1, 2024
6e715ec
test: add more cases
JakobJingleheimer Oct 1, 2024
458dcca
fix: provide missing `pjsonPath` for `GetNearestRawParentPackageJSON`
JakobJingleheimer Oct 1, 2024
903d201
fix: correct internal bindings type decs
JakobJingleheimer Oct 1, 2024
b567e9a
fixup: remove unnecessary string manipulation
JakobJingleheimer Oct 1, 2024
ed4db64
fixup: de-lint
JakobJingleheimer Oct 1, 2024
9a4b1d1
this is awful. why does anyone want this lint rule
JakobJingleheimer Oct 1, 2024
c23c8f1
fixup: correct yaml version needle
JakobJingleheimer Oct 2, 2024
c865d55
fixup: americanize name
JakobJingleheimer Oct 2, 2024
a9bf393
fixup: mangle md
JakobJingleheimer Oct 3, 2024
190e315
fixup: mangle c++ code
JakobJingleheimer Oct 3, 2024
a9e3ded
fixup: specific → nondescript to satisfy linting
JakobJingleheimer Oct 4, 2024
926b2b0
fixup: correct return dec in md
JakobJingleheimer Oct 5, 2024
485b1e3
fixup: remove temp fields & correct type decs
JakobJingleheimer Oct 5, 2024
f824ce9
fixup: wordsmith doc & expand examples
JakobJingleheimer Oct 5, 2024
7deeb7f
fixup: leverage validate* utils
JakobJingleheimer Oct 5, 2024
cf49f71
fixup: `ToString` → `ToStringView `
JakobJingleheimer Oct 5, 2024
358486c
fixup: support file URL strings, update arg name, add test case
JakobJingleheimer Oct 5, 2024
d9bdf23
fixup: damn this limited docs types validator
JakobJingleheimer Oct 5, 2024
bf5265d
fixup: require url → internal/url
JakobJingleheimer Oct 5, 2024
3e1cda8
fixup: pjson.name is optional
JakobJingleheimer Oct 6, 2024
da29815
fixup: remove unnecessary cost for default value object instantiation
JakobJingleheimer Oct 6, 2024
b531497
fixup: throw on relative `statLocation` with remediation instructions
JakobJingleheimer Oct 6, 2024
eef2b60
fixup: add test-case for require() resolving via package.json
JakobJingleheimer Oct 7, 2024
395cfa6
fixup: update consumption of `readPackage` & `read()`
JakobJingleheimer Oct 7, 2024
841848f
Revert "fixup: update consumption of `readPackage` & `read()`"
JakobJingleheimer Oct 8, 2024
945c207
fixus: restore return signature of jpsonReader.read
JakobJingleheimer Oct 8, 2024
4adac4c
fixup: null proto for read return
JakobJingleheimer Oct 9, 2024
0064379
fixup: `!== null` → `!= null`
JakobJingleheimer Oct 9, 2024
eed739c
fixup: spread with `__proto__: null`
JakobJingleheimer Oct 9, 2024
864b98a
fixup: add `exists` to `DeserializedPackageConfig`
JakobJingleheimer Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixup: de-lint
  • Loading branch information
@JakobJingleheimer
JakobJingleheimer committed Oct 1, 2024
commit ed4db64d594966b8eaf30f9510f4aa33e5c0adf9
10 changes: 4 additions & 6 deletions lib/internal/modules/package_json_reader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,9 @@ const {
ArrayIsArray,
JSONParse,
ObjectDefineProperty,
StringPrototypeLastIndexOf,
StringPrototypeSlice,
} = primordials;
const modulesBinding = internalBinding('modules');
const { resolve, sep } = require('path');
const { resolve } = require('path');
const { kEmptyObject } = require('internal/util');
const {
codes: {
Expand Down Expand Up @@ -131,7 +129,7 @@ function getNearestParentPackageJSON(startPath, everything = false) {
throw new ERR_INVALID_ARG_TYPE('startPath', 'string', startPath);
}
if (
everything != undefined &&
everything !== undefined &&
typeof everything !== 'boolean'
) {
throw new ERR_INVALID_ARG_TYPE('everything', 'boolean', everything);
Comment thread
JakobJingleheimer marked this conversation as resolved.
Resolved
Expand All @@ -143,10 +141,10 @@ function getNearestParentPackageJSON(startPath, everything = false) {
return {

@avivkeller avivkeller Oct 9, 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.

Does this also need to be null prototyped

@JakobJingleheimer JakobJingleheimer Oct 9, 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.

@aduh95 please advise 🙏

@aduh95 aduh95 Oct 10, 2024

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.

no, it's user-facing

data: {
__proto__: null,

@aduh95 aduh95 Oct 4, 2024

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.

Having null-prototype object returned to the object feels weird

@JakobJingleheimer JakobJingleheimer Oct 4, 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.

I agree. I did it for consistency between everything (not everything is used internally and needs the null prototype).

@aduh95 aduh95 Oct 10, 2024

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.

That feels like another reason not to expose everything and let the user parse the JSON themselves. We should be freezing it to avoid folks mutating it – but really, why do we bother? The API would be much simpler if it only returns a path

@JakobJingleheimer JakobJingleheimer Oct 10, 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.

The reason is because we've already done the work, and handled the caching. Doing it this way also provides better performance. Users having to do exactly the same thing is the whole reason we as engineers create utilities, and there is basically no chance any user would not subsequently parse it.

Freezing seems like a good idea.

...JSON.parse(result[0]),
...JSONParse(result[0]),

@anonrig anonrig Oct 4, 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.

This makes an unnecessary deep copy.

@JakobJingleheimer JakobJingleheimer Oct 5, 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.

I think I need to spread here in order to void the prototype. Or is voiding the prototype here not necessary because it came from c-land?

},
path: result[1],
}
};
}

const result = modulesBinding.getNearestParentPackageJSON(startPath);
Expand Down
8 changes: 6 additions & 2 deletions src/node_modules.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,9 +322,13 @@ void BindingData::GetNearestRawParentPackageJSON(
Realm* realm = Realm::GetCurrent(args);
BufferValue path_value(realm->isolate(), args[0]);

ToNamespacedPath(realm->env(), &path_value); // Required for long paths in Windows
// Required for long paths in Windows
ToNamespacedPath(realm->env(), &path_value);

auto package_json = TraverseParent(realm, std::filesystem::path(path_value.ToString()));
auto package_json = TraverseParent(
realm,
std::filesystem::path(path_value.ToString())
);

if (package_json != nullptr) {
Local<Value> result[2] = {
Expand Down
12 changes: 6 additions & 6 deletions test/parallel/test-get-package-json.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

require('../common');
const fixtures = require('../common/fixtures');
const assert = require('assert/strict');
const assert = require('assert');
const path = require('path');
const { getPackageJSON } = require('module');

Expand All @@ -15,7 +15,7 @@ const { getPackageJSON } = require('module');
}

{ // Throw when `everything` is not a boolean
for (const invalid of ['', {}, [], Symbol(), 1, 0, ]) assert.throws(
for (const invalid of ['', {}, [], Symbol(), 1, 0]) assert.throws(
() => getPackageJSON('', invalid),
{ code: 'ERR_INVALID_ARG_TYPE' }
);
Expand All @@ -28,7 +28,7 @@ const pkgType = 'module'; // a non-default value
const pathToDir = fixtures.path('packages/root-types-field/');
const pkg = getPackageJSON(pathToDir);

assert.deepEqual(pkg, {
assert.deepStrictEqual(pkg, {
path: path.join(pathToDir, 'package.json'),
data: {
__proto__: null,
Expand All @@ -42,7 +42,7 @@ const pkgType = 'module'; // a non-default value
const pathToDir = fixtures.path('packages/root-types-field/');
const pkg = getPackageJSON(pathToDir, true);

assert.deepEqual(pkg, {
assert.deepStrictEqual(pkg, {
path: path.join(pathToDir, 'package.json'),
data: {
__proto__: null,
Expand All @@ -57,7 +57,7 @@ const pkgType = 'module'; // a non-default value
const pathToDir = fixtures.path('packages/nested-types-field/');
const pkg = getPackageJSON(pathToDir);

assert.deepEqual(pkg, {
assert.deepStrictEqual(pkg, {
path: path.join(pathToDir, 'package.json'),
data: {
__proto__: null,
Expand All @@ -75,7 +75,7 @@ const pkgType = 'module'; // a non-default value
const pathToDir = fixtures.path('packages/nested-types-field/');
const pkg = getPackageJSON(pathToDir, true);

assert.deepEqual(pkg, {
assert.deepStrictEqual(pkg, {
path: path.join(pathToDir, 'package.json'),
data: {
__proto__: null,
Expand Down