-
-
Notifications
You must be signed in to change notification settings - Fork 763
fix(oxfmt): Remove jsonc parser override for (j|t)sconfig(.*)?.json
#16762
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
fix(oxfmt): Remove jsonc parser override for (j|t)sconfig(.*)?.json
#16762
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
jsonc parser override for (j|t)sconfig(.*)?.json
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.
Pull request overview
This PR attempts to remove special-case handling for tsconfig.json and jsconfig.json files, which were previously explicitly mapped to use the "jsonc" parser. The intent appears to be simplifying the code by relying on generic extension-based parsing logic instead.
Key changes:
- Removed special handling that forced
tsconfig.*.jsonandjsconfig.*.jsonfiles to use the "jsonc" parser - Removed corresponding test cases for tsconfig/jsconfig files
- Updated a comment to remove the specific tsconfig.json example
However, this change introduces a critical bug that will cause tsconfig.json files to be formatted incorrectly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
apps/oxfmt/src/core/support.rs |
Removes special handling for tsconfig/jsconfig files and their test cases; files will now use generic JSON parser based on .json extension |
apps/oxfmt/src/core/format.rs |
Updates comment to remove tsconfig.json example and generalize the explanation |
Comments suppressed due to low confidence (1)
apps/oxfmt/src/core/support.rs:85
- Removing the special handling for tsconfig.json and jsconfig.json files will cause them to be formatted with the "json" parser instead of "jsonc". This is incorrect because these files commonly contain JSONC features like comments and trailing commas, which are not valid in standard JSON.
The test fixture at test/fixtures/external_formatter/tsconfig.dummy.json demonstrates this - it contains a comment // Allow comments! and trailing commas, which are valid JSONC syntax. With this change, tsconfig.json files will be parsed with the "json" parser (matching on the .json extension at lines 81-85), which will fail to handle these JSONC features correctly.
TypeScript's official documentation explicitly allows comments and trailing commas in tsconfig.json files, making them JSONC files rather than strict JSON files.
if JSON_FILENAMES.contains(file_name) {
return Some(FormatFileStrategy::ExternalFormatter { path, parser_name: "json" });
}
if let Some(ext) = extension
&& JSON_EXTENSIONS.contains(ext)
{
return Some(FormatFileStrategy::ExternalFormatter { path, parser_name: "json" });
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e095eac to
1323c0e
Compare
Merge activity
|
1323c0e to
2577814
Compare
|
Does this close #16637, seems like it remained open? |

Fixes #16637