-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Error rework #4765
Error rework #4765
Conversation
There's something crazy going on with Travis... It's installing node 0.10. Not sure why it started doing that... I hate CI configuration. 🙄 |
4959bd5
to
a96f706
Compare
Adds error checking utilities: , , , , .
BREAKING CHANGE: ArgumentOutOfRangeError message is now just `'out of range'`, use `isOutOfRangeError` util to identify.
To test for empty errors, use `isEmptyError`.
Use `isObjectUnsubscribedError` to test for this error type.
Use `isTimeoutError` to test. BREAKING CHANGE: TimeoutError message is now `observable timed out`.
To test for errors that have occurred during teardown, use `isTeardownError`
*/ | ||
export function createRxError(message: string, code: RxErrorCode, ErrorType: any = Error) { | ||
const result = new ErrorType('RxJS: ' + message); | ||
(result as any).__rxjsErrorCode = code; |
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.
Would there be any advantage in using a symbol instead of a string key for the __rxjsErrorCode
property?
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.
There would be one disadvantage, which is it wouldn't work in IE. that's really it.
internal refactor
ObjectUnsubscribed = 2, | ||
Timeout = 3, | ||
Teardown = 4, | ||
} |
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.
We cannot use const enums. It will break with isolated modules. They cannot even be exported within internal modules, IIRC.
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.
IMO, we should get this - or something very much like it - merged: #4677
I missed the const enum in my first review. And that's something that's likely to be overlooked again and again.
I'm going to close this for now. We still want to do something like this, but I'm not sure 7.0 is the time to do it |
The first step in getting rid of custom errors to help reduce bundle sizes. Please see design doc here.