🐛 Parsing JSON files with comments fails

[mcmxcdev]

Environment information

CLI:
  Version:                      12.0.0
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           linux

Environment:
  ROME_LOG_DIR:                 unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v18.14.1"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "pnpm/7.29.3"

Rome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           true
  Linter disabled:              false
  Organize imports disabled:    true

Workspace:
  Open Documents:               0

Discovering running Rome servers...

Server:
  Status:                       stopped

What happened?

According to https://github.com/rome/tools/blob/main/CHANGELOG.md#other-changes-1, there were changes in rome v12 to resolve JSON files.

When migrating from rome v11 to v12, we encounter parsing failures for e.g. tsconfig.base.json and .eslintrc.json, due to comments in these files.

tsconfig.json is basically jsonc since https://github.com/microsoft/TypeScript/pull/5450, so it would be good for rome to allow it as well.

Especially our eslint config file has a huge amount of comments to explain why certain rules are disabled or in which case they should be enabled. Although the eslint config could be rewritten as .eslintrc.js, we use nx which generates all eslint files as JSON, so we want to stay in-sync with that.

Alternatively we could ignore all .json files which are part of the monorepo, since we don't use rome for formatting purposes.

$ pnpm rome check . 
./tsconfig.base.json:47:3 parse ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ expected a property but instead found '// Comment'
  
    45 │   },
    46 │   "exclude": ["node_modules", "tmp"],
  > 47 │   // Comment
       │   ^^^^^^^^^^
    48 │   "files": ["node_modules/jest-extended/types/index.d.ts"],
    49 │   "ts-node": {
  
  ℹ Expected a property here
  
    45 │   },
    46 │   "exclude": ["node_modules", "tmp"],
  > 47 │   // Comment
       │   ^^^^^^^^^^
    48 │   "files": ["node_modules/jest-extended/types/index.d.ts"],
    49 │   "ts-node": {

Expected result

Comments within JSON should be ignored.

Code of Conduct

• [X] I agree to follow Rome's Code of Conduct

[ematipico]

We could change our parsing strategy for tsconfig.json and jsconfig.json, but I don't think Rome should distance too much from correct JSON files.

If you use VSCode, JSONC comes out of the box. What about people that don't use VSCode? What about users that prefer to stick with just JSON?

I think this is not a bug, because Rome works as expected. I think there's room for improvement, but I think blindly applying JSONC to all JSON files might be too much. Do you have any other suggestions? Some configuration?

[mcmxcdev]

Yeah, I understand the part of wanting to stay close to correct parsing of JSON files! It might be good to add a note to the migration guide that existing JSON files could fail the lint process with v12.

I don't see any downside to using JSONC, since it gives the user more possibilities, but you can still stick to JSON only if you want.

We solved this for now by adding *.json to linter.ignore

[Conaclos]

Ideally, TSC should support jsonc extension. However, according to https://github.com/microsoft/TypeScript/issues/43121 this is not on their agenda.

A possible workaround could be to use tsconfig.jsonc and a tsconfig.json without comments that extends tsconfig.jsonc:

{
  "extends": "./tsconfig.jsonc",
}

// tsconfig.jsonc
{
  // A config file with a comment
  ...
}

[ematipico]

I could see Rome applying jsonc parsing for tsconfig.json and jsconfg.json but not for other standard JSON files.

For example, eslint and prettier don't support comments in their JSON files.

[nstepien]

IMHO Rome should assume JSONC by default.

On hand hand, we can't assume that every .json file is JSONC, as that may break other code. On the other hand, we can't assume that every .json file is JSON, as Rome would then break CI on JSONC files. So I think Rome shouldn't be a blocker here and just assume JSONC, I think it's fair to leave the responsibility of handlings comments to users. If there are comments where there shouldn't be, then other tools/code will fail, and that's fine, that's expected. It's always the user's responsibility to use the correct file extension.

Additionally, Rome could provide some settings to enforce JSON/JSONC for given paths, if there is demand for it.

[nstepien]

One more thing: VSCode already adds red squiggles under comments in JSON files, so in that regard, and for that editor, it's already covered.

[IWANABETHATGUY]

@ematipico would you mind assigning this task to me?

[IWANABETHATGUY]

https://github.com/rome/tools/blob/main/npm/rome/configuration_schema.json#L395, I think we could add a new configuration options, something like:

"json": {
  "allowCommentsWithJson": ["<here is the glob you want to match>"] 
}

then, we could give use enough ability to control, which kinds of JSON file are assumed as json with comments, if user wants to assume all json as jsonc, just put a "*" inside allowCommentsWithJson. This method also won't break user who doesn't want to assume json as jsonc

[ematipico]

I like the idea, few thoughts:

• tsconfig.json and jsconfig.json should be known files that can contain comments;
• I would name the new property allowComments (we are already inside the json section);
• The new option can accept a boolean (true) to apply the option to all *.json files, or an array of patterns;

[ChristopherHaws]

Would also be nice if it could attempt to read the files.associations config from .vscode/settings.json file to see which files are configured for jsonc. I know this would be vscode specific code, but it would avoid needing to define this config in multiple places. :)