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

Automatically added comments in JSON files cause errors in other tooling down the line. #10525

Closed
mythril opened this issue Aug 10, 2023 · 6 comments

Comments

@mythril
Copy link

mythril commented Aug 10, 2023

Describe the bug

The JSON spec does not include comments, and some other tooling breaks because of these comments.

Reproduction

Here's a tool that breaks parsing the default jsconfig.json:

$ npx apply --no-ssh rossyman/svelte-add-jest

Logs

[ info ]  Applying preset rossyman/svelte-add-jest.
[ question ]  Enable Jest DOM support? (Y/n) · true
[ question ]  Enable TypeScript support? (y/N) · true
[ question ]  Generate example test file? (Y/n) · true
[ question ]  Enable JSDOM environment by default? (Y/n) · false
[ info ]  Adding required dependencies
[ info ]  Initializing Jest config
[ info ]  Initializing Babel config
[ info ]  Enabling Jest DOM Support
[ info ]  Modifying TypeScript config for project
[ error ]  Could not read the JSON file.
SyntaxError: Unexpected token / in JSON at position 382
    at JSON.parse (<anonymous>)
    at EditJsonHandler.getContent (/home/user/.npm/_npx/1af5c665402bf7e1/node_modules/apply/dist/Handlers/EditJsonHandler.js:62:50)
    at EditJsonHandler.handle (/home/user/.npm/_npx/1af5c665402bf7e1/node_modules/apply/dist/Handlers/EditJsonHandler.js:28:42)
    at PresetApplier.performActions (/home/user/.npm/_npx/1af5c665402bf7e1/node_modules/apply/dist/Applier/PresetApplier.js:78:27)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async PresetApplier.run (/home/user/.npm/_npx/1af5c665402bf7e1/node_modules/apply/dist/Applier/PresetApplier.js:34:9)
    at async CommandLineInterface.apply (/home/user/.npm/_npx/1af5c665402bf7e1/node_modules/apply/dist/IO/CommandLineInterface.js:57:16)
    at async CommandLineInterface.run (/home/user/.npm/_npx/1af5c665402bf7e1/node_modules/apply/dist/IO/CommandLineInterface.js:54:16)

System Info

System:
    OS: Linux 5.15 Ubuntu 22.04.3 LTS 22.04.3 LTS (Jammy Jellyfish)
    CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
    Memory: 49.77 GB / 62.44 GB
    Container: Yes
    Shell: 5.1.16 - /bin/bash
  Binaries:
    Node: 18.17.0 - /usr/bin/node
    npm: 9.6.7 - /usr/bin/npm
  Browsers:
    Brave Browser: 115.1.56.20
    Chromium: 115.0.5790.110
  npmPackages:
    @sveltejs/adapter-auto: ^2.0.0 => 2.1.0 
    @sveltejs/adapter-static: ^2.0.2 => 2.0.3 
    @sveltejs/kit: ^1.20.4 => 1.22.4 
    svelte: ^4.0.0 => 4.1.2 
    vite: ^4.3.6 => 4.4.8

Severity

annoyance

Additional Information

Marked as annoyance, but it could be fairly opaque to someone who doesn't already know about JSON comments and what files are involved in the offense.

@mythril
Copy link
Author

mythril commented Aug 10, 2023

Only reason I didn't offer a pull request is that I have no idea what would be a good way to preserve the info in the comments in an accessible form for this project.

@gtm-nayan
Copy link
Contributor

Typescript along with a few other tools don't completely follow the JSON spec and allow comments in their configuration files. Even their default tsconfig on init is filled with comments so the onus is on the tooling to support those.

FWIW those comments you point to are in the template so you can just remove them in your repo without needing any change here.

@Conduitry
Copy link
Member

I agree with @gtm-nayan on this one. TypeScript and other big tools have already basically decided that their JSON files are actually JSON+comments, regardless of how polite of a thing that was to do. At this point, it's the responsibility tools like rossyman/svelte-add-jest to support comments in the JSON it's trying to parse. If you want to use tools that don't support comments, you'll just need to edit your JSON files before.

@Conduitry
Copy link
Member

I see in rossyman/svelte-add-jest#29 that that project has declared parsing JSON-C a won't-fix, and they explicitly mention it in their readme.

@ghostdevv
Copy link
Member

tsconfig.json doesn't follow the JSON spec on more than just comments unfortunately, it also has stuff like trailing commas. You could try convincing their dependency to use something like tsconfck

@ghostdevv
Copy link
Member

I'll close this for now 🙏

@ghostdevv ghostdevv closed this as not planned Won't fix, can't repro, duplicate, stale Aug 12, 2023
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

No branches or pull requests

4 participants