no useful error info from plugins emitted in API watch events

[schlawg]

Rollup Version

3.2.3

Operating System (or Browser)

macOS & fedora 36, node

Node Version (if applicable)

16.5 & 17.7.1

Link To Reproduction

https://github.com/schlawg/rollup-plugin-error

Expected Behaviour

Javascript API rollup.watch should provide specifics sourced from typescript plugins when it emits ERROR events. This should include message, frame, location, and filename. I noticed that rollup-plugin-typescript2 does provide this information (in some form) when error.ts/error is called and the specifics survive in the exception caught and thrown by rollup watch's internals before reaching the watch emitter. Maybe it's not in the proper form and that's why it's stripped?

Actual Behaviour

Events from the rpt2 plugin used in the minimal repro link look like:

"error": {
  "code": "PLUGIN_ERROR",
  "plugin": "rpt2",
  "hook": "buildEnd",
  "id": "...",
  "watchFiles": [ ]
},

There is no message, frame, or TS error code provided, but this information is clearly visible if you dump the error object from rpt2 to stdout in error.ts error function.

[lukastaegert]

This should be an issue for @rollup/plugin-typescript, please open an issue in the rollup/plugins repository instead if you have not done so already. ah sorry, misread the issue. I guess a PR would be welcome.

[TrickyPi]

I guess the problem casued by the following code:
rollup-plugin-typescript2:
https://github.com/ezolenko/rollup-plugin-typescript2/blob/f6db59613a66f58c48310aa8fa785951970b5d6d/src/index.ts#L315-L320

// workaround: err.stack contains err.message and Rollup prints both, causing duplication, so split out the stack itself if it exists (c.f. https://github.com/ezolenko/rollup-plugin-typescript2/issues/103#issuecomment-1172820658)
const stackOnly = err.stack?.split(err.message)[1];
if (stackOnly)
	this.error({ ...err, message: err.message, stack: stackOnly });
else
	this.error(err);

rollup:

rollup/src/utils/error.ts

Line 17 in 243eba5

base = Object.assign(new Error(base.message), base);

When executing Object.assign(new Error(base.message), base), the base.stack value will mess up the new Error output, because this base.stack does not contain message info, see normal stack value https://nodejs.org/dist/latest-v18.x/docs/api/errors.html#errorstack which contain message info.

Btw, the purpose of rollup-plugin-typescript2 code above is to fix cli error, and i thinks it's a rollup bug:

rollup/cli/logging.ts

Line 33 in 243eba5

stderr(dim(error.stack));

relative issues:
rollup/plugins#376
ezolenko/rollup-plugin-typescript2#103

Back to this issue, I think it's not a rollup bug, we don't need to change the error function in rollup/src/utils/error.ts, but we need to fix handleError function in /cli/logging.ts.
@lukastaegert what do you think?

[lukastaegert]

Ah yes, I guess you are right. I thought the issue was something else. But if in the reproduction you log error.error.message, you actually get all the information that the plugin includes.
I think it would be ok to put a similar logic into rollup CLI to remove the error message from the stack trace if the stack trace starts with the error message. Then rpt2 can remove its own workaround, fixing this issue.

[TrickyPi]

@lukastaegert It could be closed.

[lukastaegert]

Will close this, but it still needs an upstream fix in Rollup-plugin-typescript2 to no longer shorten stack traces.

[agilgur5]

Thanks for fixing this in #4749 @TrickyPi ! I meant to PR some changes upstream here to logging.ts, but did not get around to that timely. Was looking at this again (one of my open tabs) to make a PR and saw that it was already fixed!

Will close this, but it still needs an upstream fix in Rollup-plugin-typescript2 to no longer shorten stack traces.

downstream*.
I actually specifically wrote the code to be forward compatible if this were fixed upstream in Rollup, per ezolenko/rollup-plugin-typescript2#373, so no change is necessary 🙂
EDIT: it looks like the way it was fixed was different than I anticipated, so not quite. see below comment

rpt2 currently supports down to Rollup 1.26.3, so we'd have to leave this in until we bump past 3.7.5.

I was looking to proactively bump rpt2's peerDep range to >=2.60 (for this.load support), so would still need that workaround for users on Rollup <3.7.5

[agilgur5]

I actually specifically wrote the code to be forward compatible if this were fixed upstream in Rollup, per ezolenko/rollup-plugin-typescript2#373, so no change is necessary 🙂

Welp, I spoke too soon. Edited that out.

we don't need to change the error function in rollup/src/utils/error.ts, but we need to fix handleError function in /cli/logging.ts.

I see the change is actually a bit different than I anticipated, so maybe rpt2 does need to bump to 3.7.5.
EDIT: Created ezolenko/rollup-plugin-typescript2#458 to remedy this

There is no message, frame, or TS error code provided, but this information is clearly visible if you dump the error object from rpt2 to stdout in error.ts error function.

I am actually a little confused by this issue. ezolenko/rollup-plugin-typescript2#373 indeed retains all the fields of the error object.

When executing Object.assign(new Error(base.message), base), the base.stack value will mess up the new Error output, because this base.stack does not contain message info, see normal stack value https://nodejs.org/dist/latest-v18.x/docs/api/errors.html#errorstack which contain message info.

That is a very nice find! Thanks for noting that down in detail as well!

I'm trying to understand why this specifically impacts watch mode only; this sounds like watch mode has different error logging than non-watch? Wouldn't this normally impact both? Or does the Object.assign just happen to occur after logging?
Only impacting watch mode may explain why I did not notice this behavior earlier as well. Or, I guess it only impacts watch events very specifically.

[TrickyPi]

Thanks for your feedback.

I am actually a little confused by this issue. ezolenko/rollup-plugin-typescript2#373 indeed retains all the fields of the error object.

The stack value is changed in this condition
https://github.com/ezolenko/rollup-plugin-typescript2/blob/976dadb53a62e32958f949b72a1c16093af2a329/src/index.ts#L330

I'm trying to understand why this specifically impacts watch mode only; this sounds like watch mode has different error logging than non-watch? Wouldn't this normally impact both? Or does the Object.assign just happen to occur after logging?

This PR fixes the issue of logging error name and message repeatedly in the handleError function. This function may be called in both modes.

[agilgur5]

The stack value is changed in this condition

Yes, but message etc weren't changed, only the stack was. As you said, the Object.assign call apparently screws that up -- great find, again.

This PR fixes the issue of logging error name and message repeatedly in the handleError function.

Yes the PR makes total sense to me. I meant the issue itself as written by OP, not the PR. The issue itself apparently only impacts "API watch events"

[TrickyPi]

The issue itself apparently only impacts "API watch events"

also impacts non-watch mode. Please see https://stackblitz.com/edit/github-rgywtn?file=build-tool_.js that forked from the reproduction of this issue, I added a new js file build-tool_.js.

[agilgur5]

Hmm interesting, build-tool_.js prints twice, the first is correct, but the second one is malformed. I see, that makes a bit more sense, thanks for providing an example!

We might need to add a similar in-line plugin to our integration test suite (or in watch mode tests) to be able to catch such a regression, definitely did not test for something like that 🤔