Skip to content
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

Closed
wants to merge 8 commits into from
Closed

Error rework #4765

wants to merge 8 commits into from

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented May 10, 2019

The first step in getting rid of custom errors to help reduce bundle sizes. Please see design doc here.

@benlesh
Copy link
Member Author

benlesh commented May 10, 2019

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. 🙄

@benlesh benlesh force-pushed the error-checkers branch 2 times, most recently from 4959bd5 to a96f706 Compare May 10, 2019 18:15
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`
spec/operators/elementAt-spec.ts Outdated Show resolved Hide resolved
spec/operators/elementAt-spec.ts Outdated Show resolved Hide resolved
spec/util/UnsubscriptionError-spec.ts Outdated Show resolved Hide resolved
src/internal/Subscription.ts Outdated Show resolved Hide resolved
src/internal/util/UnsubscriptionError.ts Show resolved Hide resolved
*/
export function createRxError(message: string, code: RxErrorCode, ErrorType: any = Error) {
const result = new ErrorType('RxJS: ' + message);
(result as any).__rxjsErrorCode = code;
Copy link
Collaborator

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?

Copy link
Member Author

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.

@benlesh benlesh requested a review from cartant May 15, 2019 18:00
ObjectUnsubscribed = 2,
Timeout = 3,
Teardown = 4,
}
Copy link
Collaborator

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.

Copy link
Collaborator

@cartant cartant May 16, 2019

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.

@benlesh
Copy link
Member Author

benlesh commented Oct 15, 2019

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

@benlesh benlesh closed this Oct 15, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants