chore(shareReplay): remove redundant variable isComplete

[josepot]

Description:

This PR removes the redundant variable isComplete from the shareReplay operator. This variable became unnecessary after this change.

[cartant]

LGTM.

[benlesh]

Great! Thank you, @josepot!

[leggechr]

This broke tests at google. I'm not entirely sure why but I don't think the behavior is exactly the same as before.

[josepot]

Hi @leggechr, I'm very sorry that this PR changed the behavior. I think that I know what's happening, my bad!

I'm a bit busy right now, but in a few hours I will get to this. I will add a new test for this use case and I will fix the code. Again, I'm very sorry that I broke this 🙏. I actually thought that this was the expected behavior.

[josepot]

I could be wrong, but I can only think of one edge-case where this change would alter the behavior, and if that's what happened, then I think that this change actually fixed a bug. I could be wrong, though 😅. Ok, so this is the only thing that I can come up with:

With the previous code: If the refCount drops to zero and the source hasn't completed, then the first thing that happens is the unsubscription from the source. If that tear-down logic causes for the source to complete while it's being unsubscribed from... 🤯 then that would set the flag isComplete to true... So, the next time that a new subscriber comes, then there is a new subscription to the source, however the flag isComplete is still set to true... But that wouldn't be accurate, right? So, in this hypothetical case, if the refCount drops to zero again but the new subscription against source hasn't completed, since the flag is saying that it did complete, then it won't unsubscribe from the source.

This is the only change of behavior that I can come up with, and if that's the case, then I think that the new code is fixing a bug?

I'm sure that I must be missing something, though.

[josepot]

Ups! @leggechr I found the issue!

Consider the following code:

        subscription = source.subscribe({
          next(value: T) {
            subject!.next(value);
          },
          error(err: any) {
            hasError = true;
            subject!.error(err);
          },
          complete() {
            subscription = undefined;
            subject!.complete();
          }
        });
      }

if the source completes synchronously upon subscription, for instance:

from([1, 2, 3, 4, 5]).pipe(
  shareReplay({ refCount: true, bufferSize: 1 })
)

then the subscription is set first to undefined and then it's set to the "closed subscription" 🤦 That's it! That's why the variable isComplete is needed. Nah, I don't want to bring that redundant variable back. It's too confusing. I would do this instead:

if (subscription && !subscription.closed && useRefCount && refCount === 0) {

I'm afraid that the reason why the tests are not catching this issue is because of #5523 😕

[leggechr]

No worries @josepot! We sync rx at head into Google to test for things like this to catch them before going out to the general public. So I'm glad we could catch it.