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.
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
module: expose
getPackageJSON
utility #55229module: expose
getPackageJSON
utility #55229Changes from 11 commits
48ab696
dcddd1e
98c72f4
d4ede54
92f2c62
b83707b
e0beefa
6e715ec
458dcca
903d201
b567e9a
ed4db64
9a4b1d1
c23c8f1
c865d55
a9bf393
190e315
a9e3ded
926b2b0
485b1e3
f824ce9
7deeb7f
cf49f71
358486c
d9bdf23
bf5265d
3e1cda8
da29815
b531497
eef2b60
395cfa6
841848f
945c207
4adac4c
0064379
eed739c
864b98a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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?
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.
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?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.
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.
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.
May I recommend a different naming and API surface?
module.findPackageJson(path, { includeContents: true })
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.
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 seteverything
totrue
. Wheneverything
is nottrue
, only the select fields we use internally are parsed. That has not changed in this PR.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.
@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 inversemodule.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:
(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
If we were to do this, I think exposing 2 different utils (
findPackageJSON
+getPackageJSON
) would be better.Check failure on line 7 in lib/internal/modules/package_json_reader.js
GitHub Actions / lint-js-and-md
Check failure on line 8 in lib/internal/modules/package_json_reader.js
GitHub Actions / lint-js-and-md
Check failure on line 11 in lib/internal/modules/package_json_reader.js
GitHub Actions / lint-js-and-md
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.
Instead of everything, can we change this to
includeContents
?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.
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.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.
includeContents
does not make sense: it will include some of the contents regardless.IntelliSense :)
I think an object makes sense when there are (or potentially will be) multiple configuration options. Here it seems verbose.
Check failure on line 134 in lib/internal/modules/package_json_reader.js
GitHub Actions / lint-js-and-md
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.
Does this also need to be null prototyped
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.
@aduh95 please advise 🙏
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.
no, it's user-facing
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.
Having null-prototype object returned to the object feels weird
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.
I agree. I did it for consistency between
everything
(noteverything
is used internally and needs the null prototype).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.
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 pathThere 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.
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.
Check failure on line 146 in lib/internal/modules/package_json_reader.js
GitHub Actions / lint-js-and-md
Check failure on line 149 in lib/internal/modules/package_json_reader.js
GitHub Actions / lint-js-and-md
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.
Q: Why does this statement doesn't hold anymore?
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.
#55229 (comment)