feat(terser): emit worker error message (+stack) to parent instance #1394
Conversation
|
@shellscape would be great if you could have a look ✌️ PS: Would be great if you could also add me to the plugin maintainers: https://github.com/orgs/rollup/teams/plugin-maintainers 😅 |
|
I'm codeowner on the whole repo so I get email notifications of everything. |
|
I will remember that ^^. Did you see the change in the last comment? |
|
@shellscape is something missing? |
packages/terser/src/type.ts
Outdated
| @@ -23,6 +23,12 @@ export interface WorkerOutput { | |||
| sourceMap?: Record<string, any>; | |||
| } | |||
|
|
|||
| export interface WorkerError { | |||
There was a problem hiding this comment.
this is pretty loose. being more constrained would probably be a good idea. Adding an error property to WorkerOutput is more semantic. error can be an object with message and stack, and then all you need do is check if data.error is truthy on line 111. that will remove some complexity I don't think is needed.
There was a problem hiding this comment.
@shellscape but that would imply that all other properties of the WorkerOutput must be nullable, in case an error occoured and the error property is set and than it would be harder to distinguish between a success and error state and infer property values or am i wrong here ?
But I also see the complexity in the current implementation for the actual simple feature
There was a problem hiding this comment.
not sure I follow:
error is nullish: no error
error is truthy: error
There was a problem hiding this comment.
sure, but the code property e.g. could then no longer be a mandatory property. All attributes of the WorkerOutput type would then have to be nullable to cover all states (success +errror)
There was a problem hiding this comment.
yes. why is that a concern?
There was a problem hiding this comment.
I like to map different states using different types and then merge them with a union type if necessary 😇 .
Even with callbacks, there are also two parameters (error, result), instead of the error being an optional property of the result parameter. The same applies to promises. But I have no problem doing it the way you suggested, if you prefer ✌️
Rollup Plugin Name:
terserThis PR contains:
Are tests included?
Breaking Changes?
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