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

fix: use unknown type for errors #5084

Closed

Conversation

OliverJAsh
Copy link
Contributor

Description:
Errors can be of any type. unknown is preferable to any, as any completely disables type checking:

declare const myError: any;
myError.some.property.that.does.not.exist // runtime error!

@@ -61,21 +61,21 @@ export type InteropObservable<T> = { [Symbol.observable]: () => Subscribable<T>;
export interface NextObserver<T> {
closed?: boolean;
next: (value: T) => void;
error?: (err: any) => void;
error?: (err: unknown) => void;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be other places to update this.

@OliverJAsh OliverJAsh marked this pull request as ready for review October 16, 2019 11:01
@kwonoj
Copy link
Member

kwonoj commented Oct 16, 2019

I recall we had discussion similar to this but can't clearly remember conclusion.

Regardless, I believe this should be breaking changes.

@benlesh
Copy link
Member

benlesh commented Nov 1, 2019

Yeah, this would be a breaking change. And I'm not sure it's correct. Sure, the type of error is "unknown", but the error could also literally be anything. This change would force everyone to explicitly define the type of error they think they're going to get, and would be a pretty massive breaking change for people.

We can discuss this at the next team meeting, but I'm going to say it's unlikely we'll be willing to introduce such a massive breakage for TS users in this next version.

@benlesh benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label Nov 1, 2019
@cartant
Copy link
Collaborator

cartant commented Nov 20, 2019

Related: #5066 (comment)

Basically, I don't think we should make changes to this until TypeScript does, too - e.g. for Promise.

@benlesh
Copy link
Member

benlesh commented Nov 20, 2019

We've decided to leave as-is, until TS does something similar with Promise's error path.

@benlesh benlesh closed this Nov 20, 2019
@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label Nov 20, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants