fix(json)!: log more robustly when JSON parsing fails

[rotu]

Rollup Plugin Name: parse-json

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

Previously, when JSON failed to parse (say it was transformed by another plugin), you would get a message like below. The json plugin fails to parse, and, because the error message doesn't match the /[\d]/ regex, you get an uninformative warning message. Even if the regex did match, it would only match the first digit, not the whole line number.

bundles test/bench/versions/benchmarks.ts, src/source/worker.ts → rollup/build/benchmarks/versions...
[!] (plugin json) TypeError: Cannot read properties of null (reading '0')
src/style-spec/reference/v8.json
TypeError: Cannot read properties of null (reading '0')
    at Object.transform (C:\Users\dan\Source\maplibre-gl-js\node_modules\@rollup\plugin-json\dist\cjs\index.js:33:57)
    at C:\Users\dan\Source\maplibre-gl-js\node_modules\rollup\dist\shared\rollup.js:22879:40

[shellscape]

What's the new error logging look like?

[rotu]

Here it is now. Note the warning is now actually a warning and not a hard crash, so all the other warnings get properly reported:

bundles test/bench/versions/benchmarks.ts, src/source/worker.ts → rollup/build/benchmarks/versions...
(!) Circular dependencies
src/style-spec/expression/parsing_context.ts -> src/style-spec/expression/compound_expression.ts -> src/style-spec/expression/parsing_context.ts
src/style-spec/validate/validate.ts -> src/style-spec/validate/validate_function.ts -> src/style-spec/validate/validate.ts
src/style-spec/validate/validate.ts -> src/style-spec/validate/validate_function.ts -> src/style-spec/validate/validate_object.ts -> src/style-spec/validate/validate.ts
...and 11 more
(!) Plugin typescript: @rollup/plugin-typescript TS2403: Subsequent variable declarations must have the same type.  Variable 'AbortSignal' must be of type '{ new (): AbortSignal; prototype: AbortSignal; abort(reason?: any): AbortSignal; timeout(milliseconds: number): AbortSignal; }', but here has type '{ new (): AbortSignal; prototype: AbortSignal; }'.
node_modules/@types/node/globals.d.ts: (72:13)

72 declare var AbortSignal: {
               ~~~~~~~~~~~

  node_modules/.pnpm/typescript@4.9.3/node_modules/typescript/lib/lib.dom.d.ts:2071:13
    2071 declare var AbortSignal: {
                     ~~~~~~~~~~~
    'AbortSignal' was also declared here.

(!) Plugin typescript: @rollup/plugin-typescript TS2322: Type 'unknown' is not assignable to type 'Serialized'.
src/util/web_worker_transfer.ts: (114:9)

114         return input;
            ~~~~~~~~~~~~~

(!) Plugin json: Could not parse JSON file
src/style-spec/reference/v8.json
package.json

[shellscape]

Shouldn't the build halt when there's a problem parsing a file though?

[rotu]

Shouldn't the build halt when there's a problem parsing a file though?

I think so too, though I wasn't bold enough to make that change until you mentioned it :-)

[shellscape]

Looks like CI got its act together.

[dominikg]

@shellscape would you mind updating the changelog to highlight the fact that this PR changed from this.warn to this.error? The former logs a warning, the latter fails the build, which isn't immediately obvious from

log more robustly

[shellscape]

you found the PR from the changelog, so the system is working as intended. did this break your build where a parsing failure was previously continuing on? and if so, what use case is there for ignoring that warning?

[dominikg]

i found it after following a link to the changelog in a PR opened by renovate to update @sveltejs/adapter-node an adapter to run sveltekit applications with nodejs.

it uses the json plugin as a dependency, so bumping that can be a minor if it was just log formatting, but adding new fails could require a breaking release for that.

I was confused why a change in logging was marked as breaking, so i found this by digging. People less careful who might attribute this to breaking some log parsers might just update and wonder why their build suddenly fails.

Note: I completely agree with the change itself, was just wondering if there is something that can be improved in documenting it in the changelog.

If you think the current entry is fine, i won't bother sending a PR, sorry for the noise.

edit: included a link

[shellscape]

thanks for raising your concern. The reason we link to pull requests in the change log is so that authors can describe a change in their own way. that gives contributors some agency. very occasionally we will step in to editorialize a little bit. I don't think we need a change right now, but if more users raise concerns we would definitely rethink that