Skip to content
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

feat(tsconfig): support tsconfig.json files with comments #826

Merged
merged 1 commit into from Sep 11, 2023

Conversation

miluoshi
Copy link
Collaborator

Built-in JSON.parse() doesn't allow json with comments, but tsconfig.json files support comments. By using json5 instead of native JSON we prevent errors when trying to parse such json files with comments

@miluoshi
Copy link
Collaborator Author

It would probably be a good idea to add a test for this as well.

@rumpl
Copy link
Member

rumpl commented Aug 22, 2023

What does tsc use to parse its tsconfig.json file? It might be a good idea to use the same thing if possible

@miluoshi
Copy link
Collaborator Author

miluoshi commented Aug 22, 2023

What does tsc use to parse its tsconfig.json file? It might be a good idea to use the same thing if possible

tsc uses jsonc-parser here. But since json5 (which this project is already using) is more widely used package and jsonc-parser supported syntax is a subset of json5 supported syntax, it made sense to me to just reuse json5.

@rumpl
Copy link
Member

rumpl commented Aug 22, 2023

Fair enough, could you just add a comment in this file saying what you just said, kinda explaining why we are using a different lib than TS. For our future selves and people coming here are reading the code :)

@miluoshi
Copy link
Collaborator Author

Sure, good idea :) I'll do it

Built-in JSON.parse() doesn't allow json with comments, but `tsconfig.json` files support comments.
By using `json5` instead of native JSON we prevent errors when trying to parse such json files with comments

Even though Typescript uses 'jsonc-parser' [here](https://github.com/microsoft/TypeScript/blob/5439c8111de50fa6ea239f95fc23c76dc868fc07/scripts/build/utils.mjs#L78-L81),
we can use json5, because it is a superset of jsonc syntax.
@miluoshi
Copy link
Collaborator Author

@rumpl updated both the code and commit body with a comment about jsonc-parser

@znarf
Copy link
Collaborator

znarf commented Aug 22, 2023

Duplicate with #806

@rumpl
Copy link
Member

rumpl commented Sep 11, 2023

I'll merge this one @znarf, I like the comment explaining what's going on :)

@rumpl rumpl merged commit 381fa30 into depcheck:main Sep 11, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants