fix(shareReplay): subscribe to subject before source subscription starts

[josepot]

Description:

Addresses #5520

[cartant]

LGTM

[cartant]

TBH, I preferred the previous test - the one with the cold, synchronous marbles source. The appealing thing with the marbles is the visual aspect. The parentheses indicate the value notifications are sync and all occur at 0 ms.

[josepot]

TBH, I preferred the previous test - the one with the cold, synchronous marbles source.

The problem with using cold is that if we run that test against master then the test should fail, but it passes 😞

I think that the scheduler does something behind the scenes that doesn't make the observable completely cold? 🤷‍♂️ @voliva was the one who came up with the idea of using from instead of cold and it did the trick (fails on master and passes after the fix)

[cartant]

My original, suggested test had an error in it: ! instead of |. A | character would represent completion. Does it fail as you'd expect if the marble source completes?

(abcd|)

If it doesn't, using from is fine, but I guess there's a bug if that's the case.

[voliva]

My original, suggested test had an error in it: ! instead of |. A | character would represent completion. Does it fail as you'd expect if the marble source completes?

Unfortunately it doesn't - It seems like the "frame 0" on a cold() doesn't get emitted synchronously when the subscription is made.

[cartant]

Yeah, that's the error I made. The source should have had | in it, not !. It should have been:

const a = cold(  '(abcd|)');

[cartant]

Unfortunately it doesn't - It seems like the "frame 0" on a cold() doesn't get emitted synchronously when the subscription is made.

Yeah, I guess that would be what's happening. Using from is fine. Thanks.

[josepot]

yep, sorry for the mess @cartant . I've opened #5522 to demonstrate that the test using cold for some reason does pass on master.

As much as I struggle with marble tests I do appreciate the value that they add by visually showing what's happening. However, in this particular case it looks like we can't use cold to test this

[cartant]

Could you add a comment that mentions the use of from is deliberate as cold("(abcd|)") synchronously emits multiple values at frame 0, but is not synchronous-upon-subscription? - so that the from source is not unwittingly replaced with a marble at a later date.

I'll have to go through the other tests later, too, as I suspect there are other places in which a source is supposed to be synchronous-upon-subscription and not just the synchronous emission of multiple notifications - e.g. here.

[josepot]

@cartant please let me know if this is good enough, or if you would like for me to change/rephrase that. (English is not my first language and I'm always trying to improve, so please don't hesitate to suggest changes to make it more understandable.