fix(diagnostics): `pretty` defaults to `true` in TS 2.9+ #372

agilgur5 · 2022-06-29T06:45:01Z

Summary

Changes the default configuration of pretty, so now pretty-printing is the default, as it is for tsc and in the TSConfig Reference.

Details

per https://www.typescriptlang.org/tsconfig#pretty
- TS 2.9 changed the default to true
  - so this is a legacy remnant of older versions of TS
this is why tsc by default pretty prints, even when pretty is unset
- only if it is explicitly set to false does it not do that
so change rpt2's diagnostic printing to be pretty by default as well
- I think this is a pretty big DX improvement, to be honest
- I always thought something was up with the error reporting, but saw that there was code for pretty, so didn't quite connect the dots until now
  - that while there was code for it, the default was wrong, and so unless I set pretty: true, I wasn't getting easier to read printing
    - and given that pretty: true is the default, that's a redundant / unnecessary option to set in tsconfig that I normally wouldn't ever re-set

Review Notes

Only thing I'm not sure of is non-tty environments, that I didn't even think of until I read the TS 2.9 release notes (as linked above). Not exactly sure if TS will handle those automatically, or if we need to add a check for that as well... 🤔

It's also a bit hard to check as well... though first priority should be local dev in either case.

References

Support for pretty-printing diagnostics was originally added in support for pretty: true? #47 , which I believe pre-dates TS 2.9

- per https://www.typescriptlang.org/tsconfig#pretty - TS 2.9 changed the default to `true`: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-9.html#--pretty-output-by-default - so this is a legacy remnant of older versions of TS - this is why `tsc` by default pretty prints, even when `pretty` is unset - only if it is _explicitly_ set to `false` does it not do that - so change rpt2's diagnostic printing to be `pretty` by default as well - I think this is a pretty _big_ DX improvement, to be honest - I always thought something was up with the error reporting, but saw that there was code for `pretty`, so didn't quite connect the dots until now - that while there was code for it, the default was wrong, and so unless I set `pretty: true`, I wasn't getting easier to read printing - and given that `pretty: true` is the default, that's a redundant / unnecessary option to set in `tsconfig` that I normally wouldn't ever re-set

agilgur5 · 2022-06-30T03:14:10Z

ah sh*t, while testing #345, it seems like this could result in duplicate errors... let me look into this before merging pl0x

agilgur5 · 2022-07-01T04:50:59Z

Ok that's not a duplicate error... or rather there's already a duplicate error:

w/o pretty:

./src/index.ts → ./dist/index.ts...
[!] (plugin rpt2) Error: /project-dir/src/difference.ts(1,36): semantic error TS6133: 'a' is declared but its value is never read.
src/difference.ts
Error: /project-dir/src/difference.ts(1,36): semantic error TS6133: 'a' is declared but its value is never read.
    at error (/project-dir/node_modules/rollup/dist/shared/rollup.js:198:30)
    at throwPluginError (/project-dir/node_modules/rollup/dist/shared/rollup.js:21936:12)
    at Object.error (/project-dir/node_modules/rollup/dist/shared/rollup.js:22659:20)
    at Object.error (/project-dir/node_modules/rollup/dist/shared/rollup.js:22113:42)
    at RollupContext.error (/project-dir/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:20259:30)
    at /project-dir/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:29527:23
    at Array.forEach (<anonymous>)
    at printDiagnostics (/project-dir/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:29500:17)
    at Object.transform (/project-dir/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:29785:17)
    at /project-dir/node_modules/rollup/dist/shared/rollup.js:22868:37

w/ pretty:

./src/index.ts → ./dist/index.ts...
[!] (plugin rpt2) Error: src/difference.ts:1:36 - error TS6133: 'a' is declared but its value is never read.

1 export default function difference(a: number, b: number) {
                                     ~
src/difference.ts:1:47 - error TS6133: 'b' is declared but its value is never read.

1 export default function difference(a: number, b: number) {
                                                ~

src/difference.ts
Error: src/difference.ts:1:36 - error TS6133: 'a' is declared but its value is never read.

1 export default function difference(a: number, b: number) {
                                     ~
src/difference.ts:1:47 - error TS6133: 'b' is declared but its value is never read.

1 export default function difference(a: number, b: number) {
                                                ~

    at error (/project-dir/node_modules/rollup/dist/shared/rollup.js:198:30)
    at throwPluginError (/project-dir/node_modules/rollup/dist/shared/rollup.js:21936:12)
    at Object.error (/project-dir/node_modules/rollup/dist/shared/rollup.js:22659:20)
    at Object.error (/project-dir/node_modules/rollup/dist/shared/rollup.js:22113:42)
    at RollupContext.error (/project-dir/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:20259:30)
    at /project-dir/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:29524:19
    at Array.forEach (<anonymous>)
    at printDiagnostics (/project-dir/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:29500:17)
    at Object.transform (/project-dir/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:29785:17)
    at /project-dir/node_modules/rollup/dist/shared/rollup.js:22868:37

The error that causes Rollup to bail gets printed out once, then the id of the file, then the error again (in gray, can't see colors here unfortunately) plus the stack trace. This might be Rollup behavior? Just with the pretty errors it's much more noticeable. And pretty outputs errors for the whole file it would seem like

agilgur5 · 2022-07-01T05:32:34Z

Oh, I literally just reproduced #103 😅😮
It previously didn't have a repro, but I guess it's just more visible when using pretty

agilgur5 · 2022-07-01T05:55:49Z

So setting abortOnError: false makes it no longer duplicate:

w/o pretty:

./src/index.ts → ./dist/index.ts...
(!) Plugin rpt2: /project-dir/src/difference.ts(1,36): semantic error TS6133: 'a' is declared but its value is never read.
src/difference.ts
(!) Plugin rpt2: /project-dir/src/difference.ts(1,47): semantic error TS6133: 'b' is declared but its value is never read.
src/difference.ts
created ./dist/index.ts in 575ms

w/ pretty:

./src/index.ts → ./dist/index.ts...
(!) Plugin rpt2: src/difference.ts:1:36 - error TS6133: 'a' is declared but its value is never read.

1 export default function difference(a: number, b: number) {
                                     ~
src/difference.ts:1:47 - error TS6133: 'b' is declared but its value is never read.

1 export default function difference(a: number, b: number) {
                                                ~

src/difference.ts
src/difference.ts
created ./dist/index.ts in 586ms

So if I'm understanding this correctly, I think Rollup re-throws an error from a plugin or something, causing this behavior.
I.e. from Rollup's perspective, there's an error printed at "plugin level" and then at "Rollup level" as well because it was fatal.
context.warn doesn't have this behavior as it doesn't get "re-thrown".

Changing the Context code to use throw instead of context.error results in the same behavior, so that doesn't fix it 😕
Perhaps catching in buildEnd might?

EDIT: Nope, the error in buildEnd can just be used to replace the message, but the duplicate behavior remains... 😕 :

./src/index.ts → ./dist/index.ts...
[!] (plugin rpt2) Error: rpt2 error
Error: rpt2 error
    at error (/project-dir/node_modules/rollup/dist/shared/rollup.js:198:30)
    at throwPluginError (/project-dir/node_modules/rollup/dist/shared/rollup.js:21936:12)
    at /project-dir/node_modules/rollup/dist/shared/rollup.js:22894:20
    at async Promise.all (index 0)
    at async /project-dir/node_modules/rollup/dist/shared/rollup.js:23670:13
    at async catchUnfinishedHookActions (/project-dir/node_modules/rollup/dist/shared/rollup.js:23172:20)
    at async rollupInternal (/project-dir/node_modules/rollup/dist/shared/rollup.js:23660:5)
    at async build (/project-dir/node_modules/rollup/dist/bin/rollup:1528:20)
    at async runRollup (/project-dir/node_modules/rollup/dist/bin/rollup:1668:21)

Note that it also removed the id that was printed before, as that is added by Rollup when it is available.

The only thing possible in buildEnd is to change an error to a warning (i.e. use this.warn on it), and then output a generic error afterward so that there is no duplication, i.e.:

./src/index.ts → ./dist/index.ts...
(!) Plugin rpt2: src/difference.ts:1:36 - error TS6133: 'a' is declared but its value is never read.

1 export default function difference(a: number, b: number) {
                                     ~
src/difference.ts:1:47 - error TS6133: 'b' is declared but its value is never read.

1 export default function difference(a: number, b: number) {
                                                ~

src/difference.ts
[!] (plugin rpt2) Error: rpt2 error (see above)
Error: rpt2 error (see above)
    at error (/project-dir/node_modules/rollup/dist/shared/rollup.js:198:30)
    at throwPluginError (/project-dir/node_modules/rollup/dist/shared/rollup.js:21936:12)
    at Object.error (/project-dir/node_modules/rollup/dist/shared/rollup.js:22659:20)
    at Object.buildEnd (/project-dir/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:29823:12)
    at /project-dir/node_modules/rollup/dist/shared/rollup.js:22868:37
    at async Promise.all (index 0)
    at async /project-dir/node_modules/rollup/dist/shared/rollup.js:23670:13
    at async catchUnfinishedHookActions (/project-dir/node_modules/rollup/dist/shared/rollup.js:23172:20)
    at async rollupInternal (/project-dir/node_modules/rollup/dist/shared/rollup.js:23660:5)
    at async build (/project-dir/node_modules/rollup/dist/bin/rollup:1528:20)

not the prettiest though, and it disguises the stack trace too, which can be useful to track down bugs in rpt2 code from logs 😕
(note: (!) is a yellow warning, while [!] is a red error)

agilgur5 · 2022-07-01T06:27:05Z

Ok, well I think I've figured it out, and this really is probably more a Rollup bug, but we can probably(?) workaround it.

What Rollup is printing out at the end is the error stack, which happens to contain the error itself, as far as I can tell.

So, with this code in buildEnd:

this.error({ message: "rpt2 error (see below)", stack: err.stack });

We get this log instead (no duplicate):

./src/index.ts → ./dist/index.ts...
[!] (plugin rpt2) Error: rpt2 error (see below)
Error: src/difference.ts:1:36 - error TS6133: 'a' is declared but its value is never read.

1 export default function difference(a: number, b: number) {
                                     ~
src/difference.ts:1:47 - error TS6133: 'b' is declared but its value is never read.

1 export default function difference(a: number, b: number) {
                                                ~

    at error (/project-dir/node_modules/rollup/dist/shared/rollup.js:198:30)
    at throwPluginError (/project-dir/node_modules/rollup/dist/shared/rollup.js:21936:12)
    at Object.error (/project-dir/node_modules/rollup/dist/shared/rollup.js:22659:20)
    at Object.error (/project-dir/node_modules/rollup/dist/shared/rollup.js:22113:42)
    at RollupContext.error (/project-dir/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:20259:30)
    at /project-dir/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:29524:19
    at Array.forEach (<anonymous>)
    at printDiagnostics (/project-dir/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:29500:17)
    at Object.transform (/project-dir/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:29791:17)
    at /project-dir/node_modules/rollup/dist/shared/rollup.js:22868:37

So it prints rpt2 error (see below) and then the stack trace, which has the full, pretty message embedded within it.

Error.stack is non-standardized; so the only issue is that this could potentially vary by runtime... We're only targeting Node, that being said, and this should be the same in all LTS Node versions (per the compat table in the link above, been around since Node 0.10 at least)

agilgur5 · 2022-07-01T06:36:50Z

Yea pretty sure this is a Rollup bug; this line prints toString and then later it also prints stack, which will also print the error message -- resulting in a duplicate -- and then print the stack trace. 😕

agilgur5 · 2022-07-02T03:38:39Z

Added a fix for the duplicate error messages / #103 in #373 . That should be merged along with this PR as this PR makes it more noticeable, but that bug already exists without pretty as well, so they're technically independent.

No changes are needed to this PR as the bug I discovered here is independent / already existed.

agilgur5 added kind: bug Something isn't working properly topic: TS version Related to a change in a TS version labels Jun 29, 2022

agilgur5 changed the title ~~fix(diagnostics): pretty defaults to true, not false~~ fix(diagnostics): pretty defaults to true, not false (in TS 2.9+) Jun 29, 2022

agilgur5 changed the title ~~fix(diagnostics): pretty defaults to true, not false (in TS 2.9+)~~ fix(diagnostics): pretty defaults to true in TS 2.9+ Jun 29, 2022

agilgur5 force-pushed the fix-pretty-default branch from 0bbecce to a903778 Compare June 29, 2022 21:27

This was referenced Jul 2, 2022

Duplicate error message (esp. when pretty: true) #103

Closed

fix(diagnostics): workaround Rollup duplicating error messages #373

Merged

ezolenko merged commit f048e77 into ezolenko:master Jul 6, 2022

agilgur5 mentioned this pull request Jul 6, 2022

fix(test): handle pretty formatting #377

Merged

agilgur5 added the kind: dx Improvements to dev experience, e.g. error messages, logging, external-facing docs, etc label Jul 28, 2022

agilgur5 deleted the fix-pretty-default branch July 2, 2023 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(diagnostics): `pretty` defaults to `true` in TS 2.9+ #372

fix(diagnostics): `pretty` defaults to `true` in TS 2.9+ #372

agilgur5 commented Jun 29, 2022 •

edited

Loading

agilgur5 commented Jun 30, 2022 •

edited

Loading

agilgur5 commented Jul 1, 2022 •

edited

Loading

agilgur5 commented Jul 1, 2022

agilgur5 commented Jul 1, 2022 •

edited

Loading

agilgur5 commented Jul 1, 2022

agilgur5 commented Jul 1, 2022 •

edited

Loading

agilgur5 commented Jul 2, 2022 •

edited

Loading

fix(diagnostics): pretty defaults to true in TS 2.9+ #372

fix(diagnostics): pretty defaults to true in TS 2.9+ #372

Conversation

agilgur5 commented Jun 29, 2022 • edited Loading

Summary

Details

Review Notes

References

agilgur5 commented Jun 30, 2022 • edited Loading

agilgur5 commented Jul 1, 2022 • edited Loading

agilgur5 commented Jul 1, 2022

agilgur5 commented Jul 1, 2022 • edited Loading

agilgur5 commented Jul 1, 2022

agilgur5 commented Jul 1, 2022 • edited Loading

agilgur5 commented Jul 2, 2022 • edited Loading

fix(diagnostics): `pretty` defaults to `true` in TS 2.9+ #372

fix(diagnostics): `pretty` defaults to `true` in TS 2.9+ #372

agilgur5 commented Jun 29, 2022 •

edited

Loading

agilgur5 commented Jun 30, 2022 •

edited

Loading

agilgur5 commented Jul 1, 2022 •

edited

Loading

agilgur5 commented Jul 1, 2022 •

edited

Loading

agilgur5 commented Jul 1, 2022 •

edited

Loading

agilgur5 commented Jul 2, 2022 •

edited

Loading