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

Prettier should format JSONC with trailing commas #15553

Closed
Zamiell opened this issue Oct 27, 2023 · 10 comments · Fixed by #15831
Closed

Prettier should format JSONC with trailing commas #15553

Zamiell opened this issue Oct 27, 2023 · 10 comments · Fixed by #15831
Assignees
Labels
help wanted We're a small group who can't get to every issue promptly. We’d appreciate help fixing this issue! locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.

Comments

@Zamiell
Copy link

Zamiell commented Oct 27, 2023

  • Right now, Prettier's default value of "trailingCommas" is "all", which is fantastic. Presumably, most people who use Prettier would agree with this, as the reasons for wanting trailing commas are compelling: better Git diffs, easier refactoring, and so on.
  • For all the same reasons that we would want trailing commas in a JavaScript/TypeScript file, we also want to have trailing commas in JSONC files.
  • However, Prettier does not insert trailing commas into JSONC files by default.

You could frame this problem as either:

  1. a bug in Prettier (since the default value of "trailingCommas" is supposed to be "all"), or
  2. a failure in Prettier parsing JSONC properly

Either way, this should be changed.


Steps to Reproduce

mkdir test
cd test
npm init --yes
npm install prettier --save-dev
echo '{ "foo": "bar" }' > test.jsonc
npx prettier test.jsonc --write
cat test.jsonc

Observe that the formatted JSONC file does not have trailing commas.


Current Hacky Workaround

Use the following custom Prettier config:

  overrides: [
    {
      files: ["**/*.jsonc"],
      options: {
        parser: "json5",
        quoteProps: "preserve",
      },
    },

Since JSONC is a subset of JSON5, this works properly. However, it is important that we also specify quoteProps: "preserve", as the default value for quoteProps for JSON5 is "as-needed", which would unquote JSON properties, making it non-valid JSONC.

Obviously, having to use a custom Prettier config is really unfortunate, as one of the main ideas of using Prettier is that you shouldn't use your own custom config, you should just use all the default values. But this bug / bad default behavior prevents that.

Also, as a semi-related note, it was proposed that the default value for quoteProps in JSON5 would be changed in #4639. If a PR for that was accepted, then we could simplify the workaround slightly, although the ideal solution is to not to have to do a workaround to begin with.


Summarizing Prior Discussion

This issue continues #5708.

The core issue is that Prettier does not have a separate parser for JSONC files, it just uses the JSON parser. Since the JSON parser works with comments, this "just works" some of the time, but crashes and burns when trailing commas are involved.

ikatyang says here that they considered making a JSONC parser, but this was confusing, as it was really just a trailing commas parser and not a comment parser. Which is definitely a valid concern!

However, five years have passed since that comment, and JSONC files have only become more prevalent in the ecosystem. For example, popular tools like CSpell (code spell checker), Knip (dead code finder), and TypeDoc (documentation generator) all support JSONC files by default ("cspell.jsonc", "knip.jsonc", and "typedoc.jsonc" respectively).

Thus, there is a more compelling reason for Prettier to support the JSONC format more robustly. To @ikatyang, and the rest of the Prettier team, can we revisit this decision? A dedicated parser for JSONC means that we can finally format JSONC files properly out of the box without having to use the dirty JSON5 hack specified above.

@fisker
Copy link
Member

fisker commented Oct 27, 2023

Maybe we should add a jsonc parser?

@Zamiell
Copy link
Author

Zamiell commented Oct 27, 2023

Yes, that's what I'm proposing in the final section of the OP. 🙏

@fisker
Copy link
Member

fisker commented Oct 27, 2023

I also have needs for this, @sosukesuzuki what do you think?

@sosukesuzuki
Copy link
Member

I think adding jsonc parser makes sense. This is because JSON and JSONC have different syntax.

@fisker fisker added the help wanted We're a small group who can't get to every issue promptly. We’d appreciate help fixing this issue! label Oct 27, 2023
@fisker
Copy link
Member

fisker commented Oct 27, 2023

PR welcome.

@fisker fisker self-assigned this Dec 19, 2023
@fisker fisker mentioned this issue Dec 19, 2023
4 tasks
@fisker
Copy link
Member

fisker commented Dec 19, 2023

To make sure I understand the jsonc parser corretly,

1, It should always use double quote.
2, It should always quote object keys.

@Zamiell

@Zamiell
Copy link
Author

Zamiell commented Dec 19, 2023

Yes sir!

@Cielquan
Copy link

Is there a way to disable the addition of trailing commas in jsonc files?

My markdownlint-cli2 config file is .markdownlint-cli2.jsonc. I use jsonc over json to add comments. markdownlint-cli2 parses jsonc files by stripping comments and then using JSON.parse() on the resulting string which now fails because of the added commas. prettier and markdownlint-cli2 run both in my QA tool-chain.

@Zamiell
Copy link
Author

Zamiell commented Jan 13, 2024

@Cielquan You could try something like this in your Prettier config:

  overrides: [
    // Revert JSONC parsing:
    // https://github.com/prettier/prettier/issues/15553
    {
      files: ["**/*.jsonc"],
      options: {
        parser: "json",
      },
    },
  ],

But what you described sounds hacky as hell and really terrible! You should definitely submit an issue/PR to markdownlint for them to use the actual official parser: https://github.com/microsoft/node-jsonc-parser

@Cielquan
Copy link

Thanks for the response. I will try the config snippet on monday.

Yeah, when I looked up the stacktrace and found out how the parsing was done I thought that too, that there has to be a better way to parse jsonc.

xTVaser added a commit to jabermony/jak-project that referenced this issue Jan 16, 2024
@github-actions github-actions bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Apr 14, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted We're a small group who can't get to every issue promptly. We’d appreciate help fixing this issue! locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants