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.
Add auto-import for the
package.json
imports
field #55015New 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
Add auto-import for the
package.json
imports
field #55015Changes from 2 commits
7c0fef4
60ec33b
faef5fd
1d49909
4e676a3
747e769
01ceb65
864b9a3
d0407a9
e092943
8f781c5
a2aa5ae
eaa8e2e
051239b
b3c5ca4
ea67766
7abf4a2
6a72a51
9903d72
9e2df94
c4df857
566e543
1f9f214
7a5c164
45194b2
8f2d0c3
8bef690
cd84162
8d68392
a9634ed
7f52af2
4aab191
c435528
4f29d3a
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.
Use
JSON.parse
for package.json files.readJson
uses the TypeScript parser to construct an AST and then converts that to an object 😵 I’ve been meaning to audit existing uses ofreadJson
and then rename it; I know some can be replaced.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.
Yeah, I saw that 😅. I was aiming to consistent with the implementation of auto-import for theSorry, I was thinking of some other code, I've updated it toexports
field. I was thinking maybe that should be left for a seperate PR to avoid more ad-hoc handling of package.json reading?JSON.parse
.I've actually checked btw and all the usages of
readJson
are for readingpackage.json
s and thereadJsonOrUndefined
function that's used byreadJson
is only used for build info so all of them can really be replaced withJSON.parse
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 that replacing everything with
JSON.parse
is generally correct, because it'll throw if the file has malformed JSON. The services project has a helper,tryParseJson
, which catches this, and that's used by itscreatePackageJsonInfo
(which then makes its package.json cache).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.
Yes,
JSON.parse
should be in a try/catch, particularly when reading a local package.json as opposed to one in node_modules. It’s virtually impossible for a node_modules package.json to be malformed so I don’t care as much there. My point was only that we shouldn’t be creating TypeScript AST nodes and issuing parse diagnostics just to get an object representation of a JSON 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.
@andrewbranch should this autocomplete#something.js
? I'm investigating some stuff and I concluded the conclusion that perhaps this should actually be just#something
.@module: nodenext
doesn't prescribe the specifier ending - and since thispackage.json
doesn't have a type it's an implied CJS file so the ending is optional.note for myself: fixing this would likely depend on usinggetModuleSpecifierEndingPreference
appropriatelyHmm, I still observe some inconsistencies elsewhere but now I concluded that this one is correct -
imports
/exports
always require extensions (regardless of the module format).