Replace `relaxed-json` with `strip-json-comments`

fregante commented 3 years ago

[manifest.json] is a JSON-formatted file, with one exception: it is allowed to contain "//"-style comments.

But RJSON is a bit more than that: https://github.com/phadej/relaxed-json#relaxed-json

Given this and that relaxed-json has a few CLI-related dependencies, I'd suggest swapping it out for the more appropriate strip-json-comments, which has no dependencies.

https://github.com/mozilla/addons-linter/issues/3601

┆Issue is synchronized with this Jira Task

fregante commented 3 years ago

In general, I'm a bit confused about this file:

https://github.com/mozilla/addons-linter/blob/872132e9661ea02423b1226a8926d1c9ce890810/src/parsers/json.js#L30

Why is esprima being used before RJSON at all? It seems that the whole .parse method should just be a plain call to parse: json => JSON.parse(stripJsonComments(json))

Or perhaps it should use strip-json-comments in conjunction with https://www.npmjs.com/package/json-parse-even-better-errors for even better errors

fregante commented 3 years ago

Side note: both espree and esprima are used in this project, so esprima should probably be dropped anyway (because eslint still loads espree )

Rob--W commented 3 years ago

RelaxedJSON is not used for parsing, but only as an attempt to get useful error messages. We can use JSON.parse, and if that fails, strip the comments and retry, and if that fails, try to extract a useful message from the original error (with line numbers for example).

For manifest.json and _locales/<locale>/messages.json, replacing the overly broad RJSON with stripJsonComments+JSON.parse should be safe, because Firefox itself already strips comments (without allowing a broader format): https://searchfox.org/mozilla-central/rev/4d90ff4537330d6b17cb956c0fadf759086d9bb7/toolkit/components/extensions/Extension.jsm#268-281

The linter also lints arbitrary JSON files:

Perhaps we can disable JSON parsing for non-standard JSON files (i.e. not used by the extension framework in a special way). That would also resolve #3680

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

fregante commented 2 years ago

So I suggest replacing esprima and RJSON in favor of a combination of:

strip-json-comments
json-parse-even-better-errors

They're both light, dependency-free and more focused, whereas the other two:

fregante commented 1 month ago

So it turns out this is really simple:

export const RJSON = {
  parse(jsonString) {
    return JSON.parse(stripJsonComments(jsonString));
  },
};

That's all that's needed… except that RJSON can detect duplicate keys and addons-linter specifically tests for them.

I don't know of a way to detect duplicate keys other than by parsing the JSON file manually (e.g via something like relaxed-json). I also tried JSON.parse's reviver function but that also skips the duplicates.

Either the duplicate detection is dropped, or we cannot drop relaxed-json :(

The unfortunate part is that relaxed-json hasn't seen updates in 5 years and it has a few heavy old deps

mozilla / addons-linter

Replace `relaxed-json` with `strip-json-comments` #3910