kosich / rxjs-proxify

Turns a Stream of Objects into an Object of Streams
MIT License
35 stars 3 forks source link

`undefined` value in a CombineLatest #6

Closed Nacryn closed 3 years ago

Nacryn commented 3 years ago

Hi there, First and foremost I want to say thanks for this lib, it looks very promising and I love the idea!

Second, we've started using it in one of our projects and while the statify(...), the subscribe or the .next(...) worked well, we encountered an issue when using a proxy in a combineLatest. Here's an example:

With a proxy in a combineLatest, it produces an undefined:

const first$ = statify('nothing');
const second$ = of('foo');

const combined$ = combineLatest([first$, second$]).pipe(tap(val => console.log('Tap: ', val)));

first$.subscribe(machin => console.log('Behavior sub: ', machin));
combined$.subscribe(subed => console.log('Combined sub: ', subed));

//> Behavior sub:  nothing
//> Tap:  (2) [undefined, "foo"]
//> Combined sub:  (2) [undefined, "foo"]

first$.next('something');

//> Behavior sub:  something
//> Tap:  (2) [undefined, "foo"]
//> Combined sub:  (2) [undefined, "foo"]

Where I would have expected it to work like with a normal BehaviorSubject:

const first$ = new BehaviorSubject('nothing');
const second$ = of('foo');

const combined$ = combineLatest([first$, second$]).pipe(tap(val => console.log('Tap: ', val)));

first$.subscribe(machin => console.log('Behavior sub: ', machin));
combined$.subscribe(subed => console.log('Combined sub: ', subed));

//> Behavior sub:  nothing
//> Tap:  (2) ["nothing", "foo"]
//> Combined sub:  (2) ["nothing", "foo"]

first$.next('something');

//> Behavior sub:  something
//> Tap:  (2) ["something", "foo"]
//> Combined sub:  (2) ["something", "foo"]

Am I missing something, or is it something I misunderstood? I would love to have your insight about that! Once again, thank you for this, and keep up the good work 👍

kosich commented 3 years ago

Hey, Mikaël! I'm really very glad you're enjoying this package! 🙂 And thank you for reporting this critical issue!

I think the bug is related to the way combineLatest internally checks whether the value provided is an Observable or not. Instead of duck-checking with a isObservable function, combineLatest uses from which checks if the value is an instance of Observable (rxjs source code)

I've prepared a fix for this and released it with a minor version update (minor because now instanceof Observable proxy returns true). Please, check out this release: rxjs-proxify 0.1.0

And here's a playground to ensure your example works as expected: https://stackblitz.com/edit/rxjs-proxify-fixing-instanceof?file=index.ts

If you'll find that the bug (or some form of it) is still there — please, feel free to reopen this issue.

Again, thanks for reporting this! 👋

Nacryn commented 3 years ago

It works like a charm!

Tested it earlier and I didn't encounter the problem anymore. So glad, because I was really looking forward to using this package, and this issue was rather blocking 😅 But now the road looks clear again.

Thank you for the swift answer, the explanation, and the fix! It's highly appreciated 🙌