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

Code is running ok, but typing is failing #4119

Closed
tonivj5 opened this issue Sep 9, 2018 · 4 comments
Closed

Code is running ok, but typing is failing #4119

tonivj5 opened this issue Sep 9, 2018 · 4 comments

Comments

@tonivj5
Copy link
Contributor

tonivj5 commented Sep 9, 2018

Bug Report

Current Behavior
Code is running like I expected, but typing is failing and I don't understand why.

Reproduction

Expected behavior
typing should not error

Environment

  • Runtime: Chrome v69
  • RxJS version: 6.3.2

Possible Solution

Additional context/Screenshots
image

@jgbpercy
Copy link

jgbpercy commented Sep 19, 2018

Hmm, I'm still trying to learn RxJS internals I could be wrong about any of this, but here's what I've found with a little poking around:

Firstly the specific type error that you get on stackblitz seems to be something to with stackblitz's TS version, or with the way the current version of the stackblitz editor deals with TS, or with the way stackblitz interacts with rxjs imports (more on that below).

If I copy the same code into native VS Code, then I get a different TS error, both in VS Code itself and when I run tsc, because that code legitimately doesn't type check. However, the issue is the partition, not the switchMap. Locally, I get this as an error on line 12:

Argument of type 'UnaryFunction<Observable<number | { id: number; }[]>, [Observable<number | { id: number; }[]>, Observable<number | { id: number; }[]>]>' is not assignable to parameter of type 'OperatorFunction<number | { id: number; }[], {}>'.
  Type '[Observable<number | { id: number; }[]>, Observable<number | { id: number; }[]>]' is not assignable to type 'Observable<{}>'.
    Property '_isScalar' is missing in type '[Observable<number | { id: number; }[]>, Observable<number | { id: number; }[]>]'.

This one makes sense, because partition returns a UnaryFunction, not an OperatorFunction, and pipe only takes OperatorFunctions as arguments. It seems like partition was broken (at least when used in pipe) with the move to pipeable operators, but the clean up work to deprecate/replace it hasn't been priority as partition isn't that widely used. See these issues:

#3797
#2995
#3807


As for the stackblitz error, I tried changing the TS version I had locally to a bunch of different versions from 2.6 to latest stable (3.0) and I couldn't repro the error from stackblitz with your code, so I think it's the imports thing...

Here's your code with the rxjs dependency version changed to 5.5.12: https://stackblitz.com/edit/typescript-xidamf
Note that we now get the correct type error for the partition.
(note the error for timestamp within the switchMap can be fixed with an explicit type param: switchMap(({users, timestamp}) => of<{ id: Number}[] | Number>(users, timestamp)) but I wanted to leave the code identical)

This is a (sort of) educated guess, but I think that because of the way imports work in rxjs 6 and the way stackblitz gets dependencies, stackblitz is seeing of and switchMap as coming from two different libs, and therefore seeing the Observable classes they rely on as two separately defined classes, which means that private and protected members will be incompatible (because they didn't come from the same declaration), despite them being identical in practice.

So I guess this can be closed as the issue with your code is not a bug, but it might be worth someone looking into the stackblitz thing? Unless I'm missing some obvious fix/workaround for it.

Here's a much more minimal repro of the issue, in any case: https://stackblitz.com/edit/typescript-7c1ctq
Edit: Weirder and weirder... Now when I open my repro, it doesn't show the issue, but if I copy-paste the ts from my stackblitz over the top of index.ts on https://stackblitz.com/edit/typescript-fiigar then the incorrect type error shows up again on line 5. So has to be some stackblitz issue with loading deps I guess.

From a quick look, I couldn't see anything specifically for this on the stackblitz side, but I did see this, which could well be related (and seems to still be an issue): stackblitz/core#477

@tonivj5
Copy link
Contributor Author

tonivj5 commented Sep 20, 2018

Thank you very much!! @jgbpercy, you clarified a lot of things to me 😄

Then, if partition operator is broken and it's a stackblitz issue too, should we close this issue?

@jgbpercy
Copy link

I think so yeah, #3797 and #3807 already exist for the clean up of partition.

@tonivj5
Copy link
Contributor Author

tonivj5 commented Sep 20, 2018

Ok, thank you 👍

@tonivj5 tonivj5 closed this as completed Sep 20, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Oct 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants