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

Allow JSON with trailing commas #5708

Closed
szhu opened this issue Jan 3, 2019 · 24 comments · Fixed by #10323
Closed

Allow JSON with trailing commas #5708

szhu opened this issue Jan 3, 2019 · 24 comments · Fixed by #10323
Labels
lang:json Issues affecting JSON locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:enhancement A potential new feature to be added, or an improvement to how we print something

Comments

@szhu
Copy link

szhu commented Jan 3, 2019

I'm looking for a way to support the following style:

  • JSON-based
  • Always quote keys
  • Always include trailing commas

This would typically be not a problem -- I should be able to set parser: json and trailingComma: all. However, #2308* disables this specific combination and I can't get around it.

Here's my use case: I want Prettier to format my VSCode settings files, which I keep in a git repo.

  • VSCode requires keys to be quoted, so I can't use the json5 parser.
  • I would like trailing commas to make diffs more readable.

Note: I originally filed this bug in another repo prettier/prettier-vscode#589

* I am aware that #2308 was created because trailing commas broke VSCode. However, times have changed and VSCode settings files now support trailing commas.

Thanks!

Prettier 1.15.3
Playground link

--parser json
--trailing-comma all

Input:

{
  //
  "a": 1,
  "b": 2,
}

Output:

{
  //
  "a": 1,
  "b": 2
}

Expected behavior:
I'd like a set of input options that doesn't make changes to the input; i.e., I want the output to also be

{
  //
  "a": 1,
  "b": 2,
}
@lydell
Copy link
Member

lydell commented Jan 3, 2019

--parser json-vscode :shudder:

@j-f1
Copy link
Member

j-f1 commented Jan 3, 2019

wow, they made their own JSON parser in the first commit, and still use it.

@szhu
Copy link
Author

szhu commented Jan 3, 2019

@lydell the problem isn't the prettier json parser, it parses trailing commas just fine. The problem is that there is an explicit override to disable trailingComma when the parser is set to json. Can we somehow make that exception a little more nuanced, or give a way to override that override?

Also, this particular syntax isn't VSCode-specific, it's a pretty common JSON superset that's used in config files. Sublime Text's config files accept comments and trailing commas as well.

@bakkot
Copy link
Collaborator

bakkot commented Jan 9, 2019

Have you considered suggesting to vscode (and whoever else) that they adopt JSON5 for their config, rather than this other intermediate thing? I don't think it would be a breaking change.

@bakkot
Copy link
Collaborator

bakkot commented Jan 9, 2019

See also #4639 (linked from the prettier-vscode issue).

@ikatyang
Copy link
Member

ikatyang commented Jan 9, 2019

Personally, I think jsonc (the language named JSON with Comments in VSCode, which is also used by TypeScript tsconfig.json) is better than JSON5 since it only adds missing features that are necessary so it's relatively simple.

I was thinking of adding a --parser jsonc to address this issue, but then I found that its name (JSON with Comments) is very confusing since it looks like JSON + comments but it's actually JSON + comments + trailing commas while our --parser json already handles JSON + comments, so the jsonc description may look something like "JSON with Comments: same parser as json, but trailing commas are allowed", which is super confusing.

@ikatyang ikatyang added type:enhancement A potential new feature to be added, or an improvement to how we print something status:needs discussion Issues needing discussion and a decision to be made before action can be taken labels Jan 9, 2019
@bakkot
Copy link
Collaborator

bakkot commented Jan 9, 2019

Since jsonc is a subset of JSON5, perhaps the json5 parser could consume JSON5 and emit jsonc? I think that's pretty much just a matter of accepting #4639, though there might be other differences in output which I'm missing.

@szhu
Copy link
Author

szhu commented Jan 9, 2019

Personally, I think jsonc (the language named JSON with Comments in VSCode, which is also used by TypeScript tsconfig.json) is better than JSON5 since it only adds missing features that are necessary so it's relatively simple.

@ikatyang yeah I think json5 is a little too over-the-top, at that point they might as well be using yaml…

@ikatyang
Copy link
Member

ikatyang commented Jan 9, 2019

Since jsonc is a subset of JSON5, perhaps the json5 parser could consume JSON5 and emit jsonc?

We did not have --printer <something> at this point so we can only use --parser jsonc to hint printer to output jsonc, and all of our JSON-like languages use the same parser (parseExpression from @babel/parser) so it's actually something like --parser js-expression --printer some-json-variant.

I think that's pretty much just a matter of accepting #4639

They are the same in theory, but accepting #4639 does not look like the same to me since we don't want to add more options (same as respecting original text).

@szhu
Copy link
Author

szhu commented Jan 9, 2019

I am suspecting that the most seamless solution might not be to change the parser property, but to change the trailingComma property, since it already does something similar. Maybe have trailingComma: es5 not include it in JSON but have trailingComma: all include it?

Some other suggestions:

  • Add a compatibility property: compatibility: ["strict-json", "es5"]
  • Add a way to override by parser, not just by syntax. Then tell everyone who uses strict json to add overrides: {parsers: "json", options: {"trailingCommas": false}}

@bakkot
Copy link
Collaborator

bakkot commented Jan 9, 2019

#4639 wouldn't require a new option, would it? It could just always output anything parsed as --parser=json5 as valid jsonc, where possible.

@szhu
Copy link
Author

szhu commented Jan 11, 2019

True, but would it be possible for future changes to json5 output to be non-jsonc-compatible?

@duailibe
Copy link
Member

duailibe commented Jan 14, 2019

It's hard to predict what changes could "break" the output but I think it's safe to assume that if the source is jsonc-compatible, the output would be too.

For example, let's imagine the multiline feature didn't exist in JSON5 and the feature was just added

The following would continue to be JSONC-compatible:

{
  "foo": "bar\nbaz",
}

But if you change your source code to use this new feature, then it woud no longer be compatible:

{
  "foo": "bar\
baz",
}

@NewFuture
Copy link

the jsonc parser of vscode https://www.npmjs.com/package/jsonc-parser

@felixfbecker
Copy link

Would also like this. It's very annoying when commenting out the last line of an array or object that prettier will remove the comma of the line before. So then when you comment the line back in, it's a syntax error because the comma is missing.

@lydell
Copy link
Member

lydell commented Feb 18, 2019

@felixfbecker That's indeed a very unfortunate side effect.

@difr
Copy link

difr commented May 12, 2019

.prettierrc

{
...
  "overrides": [
    {
      "files": ["*.json", ".*rc"],
      "options": {
        "parser": "json5",
        "quoteProps": "preserve",
        "singleQuote": false,
        "trailingComma": "all",
      },
    },
  ],
}

:)

@NewFuture
Copy link

@difr thanks 👍

@difr
Copy link

difr commented May 12, 2019

@NewFuture commapeople should be tight together :),

@felixfbecker
Copy link

This override works for me mostly:

overrides: {
      files: '{**/.vscode/*.json,**/tsconfig.json,**/tsconfig.*.json}',
      options: {
        parser: 'json5',
        quoteProps: 'preserve',
        singleQuote: false,
        trailingComma: 'all',
      },
    },

Except that it fails if a string contains an escaped double quote \" because then Prettier will use single quotes for the string:

If the number of quotes outweighs the other quote, the quote which is less used will be used to format the string - Example: "I'm double quoted" results in "I'm double quoted" and "This "example" is single quoted" results in 'This "example" is single quoted'.

Also, if for some reason you remove quotes around a key, Prettier won't add them back because there is no quoteProps: "always".

It would be great if there was a singleProps: "always" and "quoteProps": "always" option.

@duailibe
Copy link
Member

duailibe commented May 14, 2019

@felixfbecker Those aren't unreasonable requests. If you're interested, would you open separate issues for them? Thanks

@szhu
Copy link
Author

szhu commented Jul 11, 2019

The json5 override doesn't work when you have strings that contain double quote characters -- to avoid escaping them, Prettier will use single quotes. See #973

Workaround: Use \u0022 instead of \"

@thorn0 thorn0 added the lang:json Issues affecting JSON label May 8, 2020
@jordanbtucker
Copy link

Hi, I'm the maintainer for JSON5.

True, but would it be possible for future changes to json5 output to be non-jsonc-compatible?

I don't understand your question since JSON5 isn't compatible with JSON with Comments. The converse is true though. JSON with Comments is a subset of JSON5. Maybe mean future changes to the output of Prettier for JSON5 documents?

It's hard to predict what changes could "break" the output but I think it's safe to assume that if the source is jsonc-compatible, the output would be too.

There will be no future changes to JSON5 (as long as I'm still maintaining the project). The spec is frozen as there aren't really too many more ES5 features to implement. (Native Date and RegExp support is not going to happen, and neither is numeric keys.)

yeah I think json5 is a little too over-the-top, at that point they might as well be using yaml…

Hey, I resent that. 😄 I do get where you're coming from though. There is quite a bit of overlap between JSON5 and YAML, and YAML has better community support. I think JSON5 is even a subset of YAML, but that was never a target. However, it's harder to write a YAML parser because it allows for so many different ways to say the same thing, which is the antithesis of JSON.

@szhu
Copy link
Author

szhu commented May 8, 2020

Hi @jordanbtucker thanks for chiming in. I'm going to preface the below by saying that although JSON5 has been mentioned a lot in this issue, I think this issue is not actually related to JSON5, and the solution might not need to involve JSON5. JSON5 is simply one of the two closest Prettier syntaxes to JSON with Comments that we have (with the other being JSON). So please know that the discussion about JSON5 here is generally not about how good JSON5 is itself but rather about its effectiveness as a solution for this issue.

True, but would it be possible for future changes to json5 output to be non-jsonc-compatible?

I don't understand your question since JSON5 isn't compatible with JSON with Comments.

I meant that even if we "supported" JSON with Comments by using JSON5 and adding options to tell Prettier to not use each JSON5 feature that isn't compatible with JSON with Comments (e.g., by adding the "key keys quoted property" as noted in the comment above mine), it's possible that a future version of JSON5 adds even more leniency and we'll be in this state again until we add an option to turn off that new feature. But, as you've said below, new features are probably not going to be added, so this isn't a big concern.

There will be no future changes to JSON5 (as long as I'm still maintaining the project). The spec is frozen as there aren't really too many more ES5 features to implement. (Native Date and RegExp support is not going to happen, and neither is numeric keys.)

This is good to hear, although I still think that it feels like a hack to support one lenient JSON syntax with another. Of course this is through no fault of JSON5.

yeah I think json5 is a little too over-the-top, at that point they might as well be using yaml…

Hey, I resent that. :smile: I do get where you're coming from though. There is quite a bit of overlap between JSON5 and YAML, and YAML has better community support. I think JSON5 is even a subset of YAML, but that was never a target. However, it's harder to write a YAML parser because it allows for so many different ways to say the same thing, which is the antithesis of JSON.

Sorry for some of the assumptions I made and thanks for clarifying -- now I understand why JSON5 was created even when YAML already exists.

@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 May 17, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang:json Issues affecting JSON locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:enhancement A potential new feature to be added, or an improvement to how we print something
Projects
None yet
Development

Successfully merging a pull request may close this issue.