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 #55229base: main
Are you sure you want to change the base?
module: expose
getPackageJSON
utility #55229Changes from 13 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
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.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.
Why did you change these lines? Meaning anything after the highlighted lines included 63-64.
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.
Because as it was before, empty fields are forced to be present but with no value:
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:
Pedantically fails because
main: undefined
is not explicitly specified.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.
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.
This makes an unnecessary deep copy.
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 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?
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)