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

Consistent, predictable json/json5 handling #24554

Closed
rarkins opened this issue Sep 20, 2023 · 9 comments · Fixed by #24612
Closed

Consistent, predictable json/json5 handling #24554

rarkins opened this issue Sep 20, 2023 · 9 comments · Fixed by #24612
Assignees
Labels
priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:in-progress Someone is working on implementation type:feature Feature (new functionality)

Comments

@rarkins
Copy link
Collaborator

rarkins commented Sep 20, 2023

Describe the proposed change(s).

Files with extension .json should use JSON.parse(), files with extension .json5 or without extension such as .renovaterc should use JSON5.parse().

Where we are allowing JSON5 for .json files today, we should change so that we warn if a .json file parses with JSON5 but not with JSON.

Then after sufficient time has passed, perhaps 2 major releases, we should remove JSON5 parsing of .json files, which will result in a Config Validation error.

We should do the same with presets too.

@rarkins rarkins added type:feature Feature (new functionality) priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:ready labels Sep 20, 2023
@viceice
Copy link
Member

viceice commented Sep 20, 2023

sounds good to me

@rarkins
Copy link
Collaborator Author

rarkins commented Sep 21, 2023

An alternative: allow VSCode-style JSONC for .json files: https://github.com/microsoft/node-jsonc-parser

But some context: microsoft/vscode#100688

@viceice
Copy link
Member

viceice commented Sep 21, 2023

interesting library. maybe it allows us to migrate config files with less changes

@rarkins
Copy link
Collaborator Author

rarkins commented Sep 21, 2023

It's all quite a mess - there's also an argument for saying if you want JSONC then use .jsonc. I think these days we can support almost unlimited json/jsonc/json5 extensions for each file name anyway.

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Sep 21, 2023

export function parseJsonWithFallback(content: string): any {
  let parsedJson: any;

  try {
    parsedJson = JSON.parse(content);
  } catch (err) {
    try {
      parsedJson = JSON5.parse(content);
      logger.warn('JSON5.parse was used to parse the JSON data. Please check your json file');
    } catch (err) {
      return null; 
    }
  }

  return parsedJson;
}

This should work, I think.
Will add this function to utils folder aand use wherever necessary(JSON5 used to parse json file).

@rarkins
Copy link
Collaborator Author

rarkins commented Sep 21, 2023

Yes, this looks good, although we maybe want to log if both parsing fails too

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Sep 23, 2023

I think throwing an error will be better, incase the content isn't parsable as that is current behaviour wherever parsing is performed.

@HonkingGoose HonkingGoose added status:in-progress Someone is working on implementation and removed status:ready labels Sep 25, 2023
@jamietanna
Copy link
Contributor

jamietanna commented Sep 26, 2023

Sounds reasonable to require JSON in .json and require JSONC/JSON5 in a different filename, it's always nicer to have this sort of thing be extra predictable, as well as nudging editors what the correct file format is.

I assume that optimizeForDisabled and other config will be updated, if needed, to handle the new filename(s)?

@rarkins
Copy link
Collaborator Author

rarkins commented Sep 26, 2023

We should use the onboardingConfigFileName value for optimizeForDisabled. This probably should be renamed too, so that it's more generic.

@HonkingGoose HonkingGoose mentioned this issue Nov 6, 2023
7 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:in-progress Someone is working on implementation type:feature Feature (new functionality)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants