module: expose `getPackageJSON` utility #55229

JakobJingleheimer · 2024-10-01T22:44:43Z

Finally got around to exposing one of the utilities we've long-discussed providing to users.

Currently, users (particularly library authors like yarn, who I believe originally requested this) have to re-implement this functionality.

I ran into this issue when building a codemod that needs to consume pjson.types / pjson.exports[…].types

JakobJingleheimer/correct-ts-specifiers#6

References:

nodejs-github-bot · 2024-10-01T22:44:49Z

Review requested:

@nodejs/loaders

doc/api/module.md

typings/internalBinding/modules.d.ts

codecov · 2024-10-02T00:17:12Z

Codecov Report

Attention: Patch coverage is 96.07843% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.40%. Comparing base (103b843) to head (a9e3ded).
Report is 40 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_modules.cc	84.21%	0 Missing and 3 partials ⚠️
lib/internal/modules/cjs/loader.js	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55229      +/-   ##
==========================================
+ Coverage   88.23%   88.40%   +0.17%     
==========================================
  Files         651      652       +1     
  Lines      183863   186639    +2776     
  Branches    35824    36069     +245     
==========================================
+ Hits       162235   165005    +2770     
+ Misses      14932    14899      -33     
- Partials     6696     6735      +39

Files with missing lines	Coverage Δ
lib/internal/modules/esm/resolve.js	`96.53% <100.00%> (ø)`
lib/internal/modules/package_json_reader.js	`100.00% <100.00%> (ø)`
lib/module.js	`100.00% <100.00%> (ø)`
src/node_modules.h	`100.00% <ø> (ø)`
lib/internal/modules/cjs/loader.js	`97.31% <88.88%> (-0.11%)`	⬇️
src/node_modules.cc	`78.96% <84.21%> (+0.85%)`	⬆️

... and 45 files with indirect coverage changes

doc/api/module.md

Co-authored-by: Aviv Keller <[email protected]>

aduh95 · 2024-10-04T12:52:33Z

lib/internal/modules/package_json_reader.js

+
+    return {
+      data: {
+        __proto__: null,


Having null-prototype object returned to the object feels weird

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

lib/internal/modules/package_json_reader.js

aduh95 · 2024-10-04T12:58:54Z

doc/api/module.md

+> Stability: 1.1 - Active Development
+
+* `startPath` {string} Where to start looking
+* `everything` {boolean} Whether to return the full contents of the found package.json


I'm not sure it is a good idea to introduce yet-another way of loading a JSON file, wouldn't it be simpler to return the path and let the user deal with reading/parsing the file?

I considered that (there's another PR I opened and closed that exposed findPackageJSON, which did exactly that). We're already reading the pjson and caching it, so IMO that would force a de-op as well as force the user to manually do what we're already doing. So, why?

I think returning the parsed data means every time we consider parsing another field in the package.json, it needs to be surfaced into this API, even though that field may not even be useful for tools, but we'll end up paying the serialization/deserialization cost even though internally only selected places need selected fields. IMO even just returning the string would be better than this. Users need this for the lookup algorithm, they tend to have other fields to parse anyway.

May I recommend a different naming and API surface?

module.findPackageJson(path, { includeContents: true })

Sorry, @joyeecheung I'm not sure I follow in your message. I think you've misunderstood the behaviour here: internally, we do not use the full version—we do use getNearestParentPackageJSON but do not set everything to true. When everything is not true, only the select fields we use internally are parsed. That has not changed in this PR.

May I recommend a different naming and API surface?

module.findPackageJson(path, { includeContents: true })

@anonrig if you recall, when we were working on this a few days ago, that did not make sense due to how the C++ worked, which is what caused me to abandon the findPackageJSON + getPackageJSON route in the first place.

Regarding "find" + includeContents, this seems counter-intuitive to me: I expect something called "find" to only locate, not retrieve. I could perhaps see the inverse

module.getPackageJSON(path, { includeContents: false })

I do not like either option though because the shape of the return is changed significantly in a foot-gun way:

getPackageJSON(path, false) // '…/package.json' getPackageJSON(path, true) // { data: {…}, path: '…/package.json' }

(I simplified the 2nd arg for brevity, not necessarily to say we shouldn't use an object)

I suppose it could always return an object for consistency, but that seems like a clumsy compromise

getPackageJSON(path, false) // { path: '…/package.json' } getPackageJSON(path, true) // { data: {…}, path: '…/package.json' }

If we were to do this, I think exposing 2 different utils (findPackageJSON + getPackageJSON) would be better.

doc/api/module.md

anonrig · 2024-10-04T18:07:51Z

lib/internal/modules/package_json_reader.js

+    ...(main != null && { main }),
+    ...(type != null && { type }),


Why did you change these lines? Meaning anything after the highlighted lines included 63-64.

Because as it was before, empty fields are forced to be present but with no value:

{ "name": "foo", "exports": "./index.js" }

{ name: 'foo' main: undefined, exports: './index.js', }

The only result is hassle and gotchas: ex 'main' in pkg → true, which is a lie ("main" is not in the package.json).

And in tests, it forces you to add extra properties that don't matter:

assert.deepStrictEqual(pkg, { name: 'foo' exports: './index.js', });

Pedantically fails because main: undefined is not explicitly specified.

lib/internal/modules/package_json_reader.js

anonrig · 2024-10-04T18:10:38Z

lib/internal/modules/package_json_reader.js

 */
-function getNearestParentPackageJSON(checkPath) {
-  const result = modulesBinding.getNearestParentPackageJSON(checkPath);
+function getNearestParentPackageJSON(startPath, everything = false) {


Instead of everything, can we change this to includeContents?

And preferably make this an object rather than a plain argument. getPackageJSON(path, true) does not help the developer, unless they look into the documentation.

includeContents does not make sense: it will include some of the contents regardless.

does not help the developer, unless they look into the documentation.

IntelliSense :)

I think an object makes sense when there are (or potentially will be) multiple configuration options. Here it seems verbose.

anonrig · 2024-10-04T18:16:05Z

typings/internalBinding/modules.d.ts

-  PackageConfig['name'],
-  PackageConfig['main'],
-  PackageConfig['type'],
+  RecognisedPackageConfig['name'],


These changes seems to be unnecessary

I disagree: PackageConfig sounds like the superset of all config options. That's not what it actually is. The potential for confusion is now much more because there are now both options (subset and superset). This rename removes the ambiguity to preclude confusion.

anonrig · 2024-10-04T18:16:48Z

typings/internalBinding/modules.d.ts

-export type PackageConfig = {
-  pjsonPath: string
+export type RecognisedPackageConfig = {
+  pjsonPath: URL['pathname']


This would resolve to string, and variable name also includes path suffix. I recommend just using string

Suggested change

pjsonPath: URL['pathname']

pjsonPath: string

We talked about this on our screenshare a few days ago, and I showed why what I did is better: Whilst ultimate, yes, it does resolve to string, not any string is acceptable, so the extra human-friendly context provides valuable info; also, IntelliSense displays it as URL['pathname'].

If we really wanted to make it correct, we could create a template literal type to properly validate via typescript.

anonrig · 2024-10-04T18:18:05Z

typings/internalBinding/modules.d.ts

+export type FullPackageConfig = RecognisedPackageConfig & {
+  [key: string]: unknown,
+}


This would make every value of FullPackageConfig unknown since unknown has higher precedence than any literal type, and I don't think we need this type in here, since it is a return value and depends on the contents of the package.json. Since we are not accessing them in node.js core, we don't need to add a type for them.

This would make every value of FullPackageConfig unknown

That's not correct: TS Playground

I don't think we need this type in here […] since we are not accessing them in node.js core

I disagree: Failing to do this causes it to lie. A lying tool is worse than no tool. Even if we're not consuming them, the extra info could still be valuable, and a lie could cause an unaware dev to make a mistake. There is no drawback to doing it correctly, so we should do it correctly.

anonrig · 2024-10-04T18:19:49Z

typings/internalBinding/modules.d.ts

  string | undefined, // exports
  string | undefined, // imports
-  string | undefined, // raw json available for experimental policy
+  RecognisedPackageConfig['pjsonPath'], // pjson file path


This change seems unrelated. experimental policy doesn't exist anymore, so we might just replace this line with raw json.

so we might just replace this line with raw json.

I'm not sure what you're talking about. The type dec was merely wrong—this element was already pjsonPath.

anonrig · 2024-10-04T18:20:31Z

doc/api/module.md

+> Stability: 1.1 - Active Development
+
+* `startPath` {string} Where to start looking
+* `everything` {boolean} Whether to return the full contents of the found package.json


May I recommend a different naming and API surface?

module.findPackageJson(path, { includeContents: true })

JakobJingleheimer added 10 commits October 1, 2024 22:58

module: expose getPackageJSON utility

48ab696

fixup: convert path_value to string before fn casting to path

dcddd1e

fixup: remove obsolete reference to findNearestPackageJSON

98c72f4

fixup: add some tests cases

d4ede54

feat: adjust getNearestParentPackageJSON().path to pjsonPath

92f2c62

feat: conditionally include non-required fields only when they exist

b83707b

fixup: remove unnecessary JSON.parse() (it's already parsed)

e0beefa

test: add more cases

6e715ec

fix: provide missing pjsonPath for GetNearestRawParentPackageJSON

458dcca

fix: correct internal bindings type decs

903d201

JakobJingleheimer requested a review from anonrig October 1, 2024 22:44

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. labels Oct 1, 2024

JakobJingleheimer added c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version. labels Oct 1, 2024

fixup: remove unnecessary string manipulation

b567e9a

JakobJingleheimer commented Oct 1, 2024

View reviewed changes

doc/api/module.md Outdated Show resolved Hide resolved

JakobJingleheimer added 2 commits October 2, 2024 01:02

fixup: de-lint

ed4db64

this is awful. why does anyone want this lint rule

9a4b1d1

RedYetiDev reviewed Oct 1, 2024

View reviewed changes

typings/internalBinding/modules.d.ts Outdated Show resolved Hide resolved

RedYetiDev added module Issues and PRs related to the module subsystem. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 1, 2024

JakobJingleheimer added 2 commits October 2, 2024 09:25

fixup: correct yaml version needle

c23c8f1

fixup: americanize name

c865d55

This comment was marked as resolved.

Sign in to view

fixup: mangle md

a9bf393

fixup: mangle c++ code

190e315

RedYetiDev reviewed Oct 3, 2024

View reviewed changes

doc/api/module.md Outdated Show resolved Hide resolved

doc/api/module.md Outdated Show resolved Hide resolved

fixup: specific → nondescript to satisfy linting

a9e3ded

Co-authored-by: Aviv Keller <[email protected]>

aduh95 reviewed Oct 4, 2024

View reviewed changes

lib/internal/modules/package_json_reader.js Outdated Show resolved Hide resolved

aduh95 reviewed Oct 4, 2024

View reviewed changes

lib/internal/modules/package_json_reader.js Outdated Show resolved Hide resolved

aduh95 reviewed Oct 4, 2024

View reviewed changes

joyeecheung reviewed Oct 4, 2024

View reviewed changes

doc/api/module.md Outdated Show resolved Hide resolved

anonrig reviewed Oct 4, 2024

View reviewed changes

JakobJingleheimer added 7 commits October 5, 2024 12:01

fixup: correct return dec in md

926b2b0

fixup: remove temp fields & correct type decs

485b1e3

fixup: wordsmith doc & expand examples

f824ce9

fixup: leverage validate* utils

7deeb7f

fixup: ToString → ToStringView

cf49f71

fixup: support file URL strings, update arg name, add test case

358486c

fixup: damn this limited docs types validator

d9bdf23

JakobJingleheimer force-pushed the feat/module-utils-getFullPJSON branch from 41163ca to d9bdf23 Compare October 5, 2024 13:59

fixup: require url → internal/url

bf5265d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: expose `getPackageJSON` utility #55229

module: expose `getPackageJSON` utility #55229

JakobJingleheimer commented Oct 1, 2024 •

edited

Loading

nodejs-github-bot commented Oct 1, 2024

codecov bot commented Oct 2, 2024 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

aduh95 Oct 4, 2024

JakobJingleheimer Oct 4, 2024

aduh95 Oct 4, 2024 •

edited

Loading

JakobJingleheimer Oct 4, 2024

joyeecheung Oct 4, 2024 •

edited

Loading

anonrig Oct 4, 2024

JakobJingleheimer Oct 5, 2024

JakobJingleheimer Oct 5, 2024

anonrig Oct 4, 2024

JakobJingleheimer Oct 5, 2024

anonrig Oct 4, 2024

anonrig Oct 4, 2024

JakobJingleheimer Oct 5, 2024

anonrig Oct 4, 2024

JakobJingleheimer Oct 5, 2024

anonrig Oct 4, 2024

JakobJingleheimer Oct 5, 2024

anonrig Oct 4, 2024

JakobJingleheimer Oct 5, 2024

anonrig Oct 4, 2024

JakobJingleheimer Oct 5, 2024

anonrig Oct 4, 2024

		...(main != null && { main }),
		...(type != null && { type }),

module: expose getPackageJSON utility #55229

Are you sure you want to change the base?

module: expose getPackageJSON utility #55229

Conversation

JakobJingleheimer commented Oct 1, 2024 • edited Loading

nodejs-github-bot commented Oct 1, 2024

codecov bot commented Oct 2, 2024 • edited Loading

Codecov Report

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduh95 Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

module: expose `getPackageJSON` utility #55229

module: expose `getPackageJSON` utility #55229

JakobJingleheimer commented Oct 1, 2024 •

edited

Loading

codecov bot commented Oct 2, 2024 •

edited

Loading

aduh95 Oct 4, 2024 •

edited

Loading

joyeecheung Oct 4, 2024 •

edited

Loading