-
Notifications
You must be signed in to change notification settings - Fork 336
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add NonRetryableError #2634
Add NonRetryableError #2634
Conversation
3fe32c6
to
3da1b5e
Compare
export class NonRetryableError extends Error { | ||
// `__brand` is needed for Workflows' engine to validate if the user returned a NonRetryableError | ||
// this provides better DX because they can just extend NonRetryableError for their own Errors | ||
public readonly __brand: string = 'NonRetryableError'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking but...This is a bit unfortunate. I'd almost prefer something like NonRetryableError.isInstance(err)
that can check an internal private... e.g.
const kTag = Symbol('kTag');
class NonRetryableError extends Error {
[kTag] = kTag;
static isInstance(value) { return value[kTag] === kTag; }
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is your suggested implementation serialized by RPC? If yes, I'm fine with replacing it 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, no, it would not be. Would you mind adding a comment then to indicate that the field is a public own property because it needs to be serialized for RPC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we use name property as a way if distinguishing this error type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we use name property as a way if distinguishing this error type?
It's done so that users can extend NonRetryableError (and override the name) for better observability and DX.
3da1b5e
to
da18180
Compare
No description provided.