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 types for catchError #5609

Closed
wants to merge 1 commit into from
Closed

Conversation

leggechr
Copy link
Contributor

Description:
The cast was removed in #5572 but it is needed in Google for this to compile.

The cast was removed in ReactiveX#5572 but it is needed in Google for this to compile
@cartant
Copy link
Collaborator

cartant commented Aug 4, 2020

@leggechr I'd kinda like to figure out why the problem is effected. What version of TS is used for the compilation? And how is the operator used? Does it differ from any of the uses in the dtslint tests?

@benlesh
Copy link
Member

benlesh commented Aug 7, 2020

Yeah, I'm very curious as to why this wouldn't compile.

@leggechr
Copy link
Contributor Author

leggechr commented Aug 7, 2020

So I actually think everywhere that this new lift operator is used the types are broken in Google for corresponding operators. It's been a while since I tried to fix these. I'm just trying to find the offending examples. Will update here once I've found them again.

@leggechr
Copy link
Contributor Author

leggechr commented Aug 7, 2020

Okay the error that is thrown in google is with the actual catchError operator itself:

.../src/internal/operators/catchError.ts:130:5 - error TS2322: Type 'Observable<unknown>' is not assignable to type 'Observable<T>'.
  Type 'unknown' is not assignable to type 'T'.
    'unknown' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint '{}'.

130     operator.caught = caught;
        ~~~~~~~~~~~~~~~
.../src/internal/operators/catchError.ts:131:5 - error TS2322: Type 'Observable<unknown>' is not assignable to type 'Observable<T | ObservedValueOf<O>>'.
  Type 'unknown' is not assignable to type 'T | ObservedValueOf<O>'.
    Type 'unknown' is not assignable to type 'ObservedValueOf<O>'.

131     return caught;
        ~~~~~~~~~~~~~~

The version of typescript is 3.8.2.

@cartant
Copy link
Collaborator

cartant commented Aug 7, 2020

It seems that, in this codebase, there is an any inference made in const operator = new CatchOperator(selector); but you are seeing unknown inferred instead. We are using a later TypeScript version: 3.9.2, so this is possibly version-related behaviour.

I'd look into this further, but it's likely that the change in this PR isn't needed, as Ben has refactored catchError - see #5627

Could you try the current master, @leggechr ?

@benlesh
Copy link
Member

benlesh commented Aug 24, 2020

Okay... so I've run into this a couple of times, and it might be because we allow AsyncIterator as an ObservableInput now. That means that if tsconfig isn't set up with a target of esnext it will not be able to figure out what type things are, because AsyncIterator doesn't exist, and you get Observable<unknown> from ObservableValueOf<T>.

@cartant
Copy link
Collaborator

cartant commented Aug 24, 2020

FWIW, es2018 should be sufficient, as AsyncIterator interface is in node_modules\typescript\lib\lib.es2018.asynciterable.d.ts. I think it's reasonable to impose a minimum lib for v7. The lib is just the types, after all.

@benlesh benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label Sep 9, 2020
@benlesh
Copy link
Member

benlesh commented Sep 9, 2020

Related to #5708

@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label Sep 9, 2020
@cartant
Copy link
Collaborator

cartant commented Mar 7, 2021

Pretty sure this is closed by #5807

@cartant cartant closed this Mar 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants