kosich / rxjs-autorun

Re-evaluate an expression whenever Observable in it emits
MIT License
34 stars 2 forks source link

Support multiple independent subscriptions of the same runner #6

Closed Jopie64 closed 4 years ago

Jopie64 commented 4 years ago

Hello,

Thanks again for this amazing work!

I tried out it's behavior when the same observable returned from run() is subscribed multiple times. I expect it to then also subscribe the upstream observables multiple times. (It should not share the subscription I think. If that is desirable, one could always use shareReplay() or something.) But currently that is not what's happening.

I created some tests in this commit: 5da93e09779827dc1efb180e1ddd93a127830d6c. Most of them fail. I also tried to make a quick fix, but failed to do so yet...

Do you agree about the new tests I made? If not, what do you think should happen when multiple subscribers subscribe the same runner?

I hope you'll look into this as well.

Greetings, Johan

Jopie64 commented 4 years ago

Ok I figured out why the tests kept failing. It was because of something in the tests themselves. They succeed now (together with the fix). Can you still take a look at this commit? 10a2d2bec3d61b3fe51d1ca6e6d41833319e69a1

kosich commented 4 years ago

Hi, Johan!

You're absolutely right! Great catch! 🙌 💯

It took me a while to understand the tests tho 😅 But they do cover everything deeply! Maybe preceding your tests with some simpler example would help, e.g:

it.only('should subscribe twice', () => {
    let count = 0;
    const o = new Observable(observer => {
        count++;
        observer.complete();
    });

    const r = run(() => $(o));
    r.subscribe();
    r.subscribe();
    expect(count).toBe(2); // it's 1 now!
})

Thank you!

Would you like to create a PR on this? 😊

Jopie64 commented 4 years ago

Good idea about the extra test!

I'm not sure what happens in github when creating a pull request of a commit on top of commits that are already in a different pull request? So that's one reason why I didn't create one already... I'll see what I can do.

kosich commented 4 years ago

Yeah, the diff would be huge. I'm sure it'll wait. We have this issue so that we wont forget about this!

Jopie64 commented 4 years ago

I added your suggestion as extra test and linked the commit to this issue.

For the PR I'll wait for #5 to complete first because this commit is dependent on it.

kosich commented 4 years ago

5 is merged 🙌

NB: that I use squash-merge, so you'll have to rebase interactively to drop commits from #5