fix: Permit single-line JSON comments #1361
Conversation
package.json
Outdated
| @@ -75,7 +75,6 @@ | |||
| "sign-addon": "0.3.1", | |||
| "source-map-support": "0.5.3", | |||
| "stream-to-promise": "2.2.0", | |||
| "strip-json-comments": "2.0.1", | |||
There was a problem hiding this comment.
@Rob--W Do you mind to bring back the "strip-json-comments" dependency in this PR?
"strip-json-comments" removes the comments stripped by replacing them with white spaces, which is nice because it ensures that any JSON parsing error is going to point to the right place of the original JSON file (which is also confirmed by the change you have to apply in the "tests/unit/test-util/test.manifest.js" test case below to make it pass).
It is true that "strip-json-comments" supports both line (// ...) and multiline comments (/* ... */) whereas Firefox only supports line comments, but it is not a big deal and web-ext lint has its own validation rules and it is reporting the expected error on a json file that contains multiline comments (whereas web-ext strips the comments only to be able to read the JSON file and retrieve some information from that file).
I would prefer #384 to ensure that an extension is completely valid before running it (which also have an open PR, #817, currently it has some conflicts but I'm going to take a look at it asap)
|
@rpl Done. |
| } catch (error) { | ||
| throw new UsageError( | ||
| `Error reading messages.json file at ${messageFile}: ${error}`); | ||
| } | ||
|
|
||
| try { | ||
| messageData = parseJSON(String(messageContents), messageFile); | ||
| messageData = parseJSON(stripJsonComments(messageContents), messageFile); |
There was a problem hiding this comment.
@Rob--W Do you mind to add an additional test case for this fix in 'tests/unit/test-cmd/test.build.js'?
Strip comments from _locales/*/messages.json. And set an explicit encoding in readFile so that the return value is a string instead of a Buffer.
|
@rpl Done again. |
Fixes #1352 .