gracefully handle error thrown from the service

[phryneas]

This needs some clarification - I'll add some in-code-comments.

PS: no need for panic, the current 1.0.3 should not break anything for anyone.
This only addresses the case where Errors are thrown within the service (which should not happen in the first place) and in that case the 1.0.3 behaviour should only be logging an "uncaught promise" warning and nothing grave should happen.
I just had hoped to get this into the release, because it was clearly missing :/

[phryneas]

Also, currently the error object is too complex to be passed over the thread boundary, so only an empty object will arrive on the parent thread. I'll address that in a second commit, just wanted to already get my questions out to you guys.

[phryneas]

Okay, errors are now correctly serialized, too.

I tried this in single-thread mode and also in cluster mode by manually throwing an Error from within the service - in both cases, the error is correctly logged.

If you are fine with passing these errors to the serviceStartError hooks, this is ready to go from my side.
Otherwise, just say the word which hook is more appropriate or if I should add a new hook ( serviceCheckError or errorWhileChecking? ) for that.

Again, sorry for the hassle - I had noticed this problem earlier today, but I had not anticipated it to be merged so quickly after @johnnyreilly was still testing, otherwise I would have notified you about this immediately.

[johnnyreilly]

no need for panic, the current 1.0.3 should not break anything for anyone.

Cool. Do you want to update the package.json and CHANGELOG.md too please? 1.0.4 should be the new version.

If you are fine with passing these errors to the serviceStartError hooks, this is ready to go from my side.
Otherwise, just say the word which hook is more appropriate or if I should add a new hook ( serviceCheckError or errorWhileChecking? ) for that.

What's super important here is that these sort of errors fail the build. We want to avoid the situation where someone builds, there's an error but the build is considered passing. That way leads to errors in production.

Given that it's only really running with async: false where errors are propogated to webpack I think we need to consider that.

I'm okay with a new hook like errorWhileChecking. I'm also okay (and perhaps leaning towards) using the existing mechanism that uses this.handleServiceMessage(result); to report issues.

Errors reported using this mechanism already get surfaced to webpack / the notifier etc. Can we consider using that? I'm mindful that it may not be a perfect fit and we shouldn't force it if it's not. But if it works then that's pretty straightforward.

[phryneas]

@johnnyreilly actually, using handleServiceMessage is a great idea!
Also, this would make most of this PR obsolete - there's already a try..catch block in service.ts that currently re-throws all errors - I guess we can just let that emit diagnostics and use the existing mechanisms to display that error. I'll take a look at that.

[phryneas]

Okay, completely different approach. Now internal errors are just reported as diagnostics, but as a sepearate type of NormalizedMessage. Everything else just happens in the formatter.

Do you think that is feasible or should those error be passed as a separate array next to diagnostics and lints? That array could be added as a third argument to the receive hook - with the downside that for now, everything out there might miss them.

[johnnyreilly]

Okay, completely different approach. Now internal errors are just reported as diagnostics, but as a sepearate type of NormalizedMessage.

I think this is good!

Do you think that is feasible or should those error be passed as a separate array next to diagnostics and lints? That array could be added as a third argument to the receive hook - with the downside that for now, everything out there might miss them.

For exactly that reason I want to treat them as diagnostics. I'm mindful of third parties consuming this and aiming for a non-breaking changes approach. I think the introduction of a 3rd type of NormalizedMessage might prove problematic given that I know that create-react-app have rolled their own formatter and this would likely create problems in that space.

I wonder if we could take your idea and tweak it a little. Instead of a new type, consider a new code. code can be a string (in the case of lints) or a number (in the case of typescript errors).

So maybe ErrorType remains like this:

export type ErrorType = 'diagnostic' | 'lint'

But in the case we're talking about we use the code: "INTERNAL_ERROR" or similar. Let me put comments on the actual code to clarify what I mean.

[johnnyreilly]

What do you think of the approach suggested below? It means even less code changes. It slightly overloads code of NormalizedMessage` but not in a way that should be problematic. (I think)

BTW once we're settled on how this should work, would you be able to add an integration test to ensure we've got this covered please?

Out of interest (and I really should have asked this question earlier 😄 ) - what happened with the pre-worker-rpc fork-ts-checker-webpack-plugin in the case of unhandled errors? How was behaviour different?

[johnnyreilly]

We won't need this anymore

[phryneas]

Unfortunately I think we still do - internal errors have no useful file, line or column to display, but they have a stack trace. So those still need to be displayed separately.

I could try to parse the file name, line and column from the stack trace, but given that's just a string that might fail in different node versions if the formatting changes or something alike. Also, a stack trace has multiple filenames/lines/columns, so without separate handling that extra information might get lost.

[johnnyreilly]

These are valid points....

Hmmmm......

I really want to avoid having another error type because of consumers like create-react-app etc rolling their own formatters.

Given that, as I understand it, this the edgiest of edge cases.... i.e we don't expect this to happen once in a blue moon then perhaps it's not the end of the world.

What's your view?

[phryneas]

I think it doesn't hurt this way. The stack is additional information and file, line and character are optional properties, so every formatter should have to check for those to exist anyways (and in the worst case, it would display error in file [internal], undefined:undefined).

With a formatter not supporting this, the users would see the error and that it occured, they might just not get all the information available.

The only thing I'm not 100% sure about is that file would currently contain [internal], which might pose problems if the formatter just tries to read the file without checking for it's existence.

[johnnyreilly]

The only thing I'm not 100% sure about is that file would currently contain [internal], which might pose problems if the formatter just tries to read the file without checking for it's existence.

I'm pretty sure it doesn't. Let's go with this.

[phryneas]

What do you think of the approach suggested below? It means even less code changes. It slightly overloads code of NormalizedMessage` but not in a way that should be problematic. (I think)

That's fine with me. Although it would still need minor modifications to the formatter to display the stack trace (I tried adding that to content, but thats not really nice with pop-up-notifications ;) ).
But since that's optional information, it would hurt nobody if they were using another formatter and just not see it.

BTW once we're settled on how this should work, would you be able to add an integration test to ensure we've got this covered please?

I can try, although I'm not sure how to forcefully make it throw an error without doing something like

if (process.env.TEST_FORCE_THROW_ERROR === "true") { throw new Error("test!"); }

in the production codebase. Would you be fine with something like that? And if yes, where would you put it?

Out of interest (and I really should have asked this question earlier smile ) - what happened with the pre-worker-rpc fork-ts-checker-webpack-plugin in the case of unhandled errors? How was behaviour different?

Before, the thread would just be gone for good without anyone ever noticing, except maybe for some noise on the console 😆
Most likely, it just never happened or nobody ever noticed it. I just noticed it because a change to an internal api of typescript caused an error to be thrown.

[johnnyreilly]

if (process.env.TEST_FORCE_THROW_ERROR === "true") { throw new Error("test!"); }
in the production codebase.
Would you be fine with something like that? And if yes, where would you put it?

No - let's not!

Most likely, it just never happened or nobody ever noticed it. I just noticed it because a change to an internal api of typescript caused an error to be thrown

Ah well this will be an improvement then!

[phryneas]

if (process.env.TEST_FORCE_THROW_ERROR === "true") { throw new Error("test!"); }
in the production codebase.
Would you be fine with something like that? And if yes, where would you put it?

No - let's not!

I assumed as much. Do you have any other idea on how to trigger this?

PS: And yeah, I thought of mocking a require, but that won't help since that will only mock on the same thread, but not on a different one :(

[johnnyreilly]

I don't. Given it's an edge case I don't think it's the end of the world if we don't have an integration test. A unit test might be doable though?

[phryneas]

@johnnyreilly actually, I found a way to do a proper integration test - using node's --require to inject a file to be loaded before service.ts and using mock-require within that file to prepare an IncrementalChecker that throws an error on nextIteration.
Okay, explaining this sounds more complicated that it is xD - see for yourself: 6a03586

Actually, I think this is might even be useful for other tests to inject some code 😃

I'll add your suggested changes for NormalizedMessage tomorrow.

[phryneas]

Okay, did as you requested and additionally, I moved the code for creating a NormalizedMessage from service.ts to NormalizedMessageFactories.ts.

Only thing that I think we can't do as you requested is this comment - I replied inline: #249 (comment)

[johnnyreilly]

Cool - I've responded to your response 😊

[phryneas]

Since there seems to be no email notification on those comments: I responded to your response to my response ;)

[johnnyreilly]

Shipped with https://github.com/Realytics/fork-ts-checker-webpack-plugin/releases/tag/v1.0.4