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

json.formatter.trailingCommas breaking package.json files #4755

Open
1 task done
hyperknot opened this issue Dec 18, 2024 · 13 comments · Fixed by #4803
Open
1 task done

json.formatter.trailingCommas breaking package.json files #4755

hyperknot opened this issue Dec 18, 2024 · 13 comments · Fixed by #4803
Assignees
Labels
A-Formatter Area: formatter S-Bug-confirmed Status: report has been confirmed as a valid bug
Milestone

Comments

@hyperknot
Copy link

hyperknot commented Dec 18, 2024

Environment information

CLI:
  Version:                      1.9.4
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

Environment:
  BIOME_LOG_PATH:               unset
  BIOME_LOG_PREFIX_NAME:        unset
  BIOME_CONFIG_PATH:            unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v20.17.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "pnpm/9.10.0"

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    false
  VCS disabled:                 false

Workspace:
  Open Documents:               0

What happened?

  1. set json.formatter.trailingCommas to "all"
  2. all non-JSONC files are broken, including package.json files

Expected result

As per the docs, Biome is able to identify JSON vs JSONC files, especially config files. It should only allow json.formatter.trailingCommas for the supported files, not for every JSON file, and definitely not for package.json files, which break.

From the docs:

For files with an extension name of .jsonc or those identified as jsonc according to the language identifier, Biome automatically applies the following default settings for parsing and formatting them:

json.parser.allowComments: true
json.parser.allowTrailingCommas: true
json.formatter.trailingCommas: none

Please note, some well-known files like tsconfig.json and .babelrc don’t use the .jsonc extension but still allow comments and trailing commas. While others, such as .eslintrc.json, only allow comments. Biome is able to identify these files and adjusts the json.parser.allowTrailingCommas option accordingly to ensure they are correctly parsed.

Because of this bug, json.formatter.trailingCommas cannot be used.

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@hyperknot hyperknot added the S-Needs triage Status: this issue needs to be triaged label Dec 18, 2024
@ematipico
Copy link
Member

This was intentional because tools like VSCode support trailing commas even in JSON files (no parsing errors). Eventually, users got used to this weird behaviour without even noticing.

To try to support these use cases, we let the user decide what they want by providing highly customisable options.

I don't mind changing this, but it would break a lot of use cases, and users won't be able to support them

@ematipico ematipico added S-Needs response Status: await response from OP S-Needs discussion Status: needs a discussion to understand criteria and removed S-Needs triage Status: this issue needs to be triaged labels Dec 18, 2024
@hyperknot
Copy link
Author

hyperknot commented Dec 18, 2024

Why not follow prettier's convention? It correctly formats both JSONC and JSON based on extension, not even a config file is needed.

Also, if biome knows tool specific config formats, then it should definitely recognize package.json files. I guess the package.json breaking is definitely a bug.

@ematipico
Copy link
Member

ematipico commented Dec 18, 2024

As I said, I don't mind changing this, but what about those uses cases where users want trailing commas in JSON files? That's the point of discussion.

@hyperknot
Copy link
Author

hyperknot commented Dec 18, 2024

Anyway, in my opinion supporting JSONC-in-JSON breaks the whole ecosystem of JSON parsing tools, it's a super dangerous thing to do for a linter. If VSCode users want commas, they should rename their files.

One solution could be:

  1. Same defaults as prettier => add trailing comma to JSONC without any config.
  2. Two ideas:
  • Add config option like json.formatter.jsoncInJson
  • Introduce separte json and jsonc formatter config.

@Conaclos
Copy link
Member

Some JSONC parsers don't support trailing commas. If I remember correctly the VSCode team recommends to not adding trailing comma in its JSON files recognized as JSONC files.

IN that case this could make json.formatter.trailingCommas pretty useless if we don't add trailing commas in JSON and JSONC files.
The proper action seems to use overrides with json.formatter.trailingCommas.

@hyperknot
Copy link
Author

A related issue I've just run into is that tsconfig.*.json is also not recognised correctly.
#4769

Prettier also gets this one perfectly.

Basically prettier really gets JSON/JSONC formatting, without any config it just works. Both on comments in JSON, both on commas in JSONC, etc. Please take it as a reference implementation for JSON formatting and follow their defaults.

@ematipico
Copy link
Member

Both on comments in JSON

This is debatable, and maybe a personal preference. Comments in JSON aren't allowed. Agree to disagree, but we prefer to follow the spec by default.

@hyperknot
Copy link
Author

Up there you stated: "This was intentional because tools like VSCode support trailing commas even in JSON files (no parsing errors). Eventually, users got used to this weird behaviour without even noticing."

I cannot follow the logic here.

@ematipico
Copy link
Member

Apologies for the misunderstanding. In that comment I explained the reason why the option exists and why it behaves like that. However, we agreed that there's a bug to fix :)

@hyperknot
Copy link
Author

OK, if the tools specific configs work correctly, then it's a great first step.

For general files, I also believe that normal JSON should be according to the spec, so no trailing comma, no comment.

For JSONC I have no idea, it's not really a format. C can mean comma and it can also mean comment, it's depending on the tool being used.

@arendjr
Copy link
Contributor

arendjr commented Dec 21, 2024

Adding the Biome 2.0 label, since I think it would be the moment to fix this if we can. Agreed it needs more careful discussion though.

Introduce separte json and jsonc formatter config.

I’m inclined to lean in this direction, but it’s still tricky. We could have a dedicated jsonc.formatter.trailingCommas option in addition to json.formatter.trailingCommas, where the default for the former is the value of the latter. But that suggestion may complicate things for our overrides behavior.

What do you think?

@arendjr arendjr added this to the Biome 2.0 milestone Dec 21, 2024
@hyperknot
Copy link
Author

hyperknot commented Dec 21, 2024

I recommend the following rule for JSON files:

  • JSON files should always be according to the spec, no trailing commas, no comments. The only exception is the magic list of config files. There should be no config option for JSON files, otherwise it'd brake the specs.

And for JSONC files:

  • JSONC files should have the trailing commas user-configurable. Ideally it'd be called jsonc.formatter.trailingCommas and not json.formatter...

@arendjr
Copy link
Contributor

arendjr commented Dec 22, 2024

Yes, I think that may be the way to go indeed.

It does take away the ability for users to override the behavior on regular JSON files, but that may not be much of an issue. In another thread we also discussed the ability to override the file type of specific files and/or extensions. So someone who really wants could always force the behavior of JSONC on JSON files anyway.

@ematipico ematipico added A-Formatter Area: formatter S-Bug-confirmed Status: report has been confirmed as a valid bug and removed S-Needs response Status: await response from OP S-Needs discussion Status: needs a discussion to understand criteria labels Dec 24, 2024
@ematipico ematipico self-assigned this Dec 29, 2024
@ematipico ematipico linked a pull request Dec 29, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Formatter Area: formatter S-Bug-confirmed Status: report has been confirmed as a valid bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants