chore(take): test case for error notification after limit is reached

[backbone87]

Description:
provides a failing test case for an edge case of take and dependent operators as presented in #5487

Will add a fix, if a change in behavior is agreed upon.

Related issue (if exists): fixes #5487

[cartant]

The behaviour reported in that issue is a bug. It's supposed to stop listening.

IMO, I would deem this a bug fix - pretty much any bug fix could be considered a breaking change - but could be persuaded that it's a break and should go into 8.x. Either way, this has to be fixed.

I have this on my of things to do/fix, but you - or anyone else - are welcome to do it. I also have noted that elementAt, find and takeWhile have - or might have? - a similar issue. If those other ops do have the bug, they should be fixed in separate PRs - for a neater CHANGELOG.

[benlesh]

Hmm... I just closed that issue as "not a bug". It seems more like a misunderstanding in the order of execution. I'll reopen because I'm interested in other opinions.

[backbone87]

while checking out take, i noticed that it also doesn't handle non-ints very well either.

imho, it should either check for Number.MAX_SAFE_INTEGER and floor the count or try to deal with all numbers (the latter can be problematic because of incrementing doesn't work past Number.MAX_SAFE_INTEGER

[benlesh]

@backbone87 ... probably a separate issue to address.

I do have a fix for take, though. Let's add a skip to this test and get it merged, and I'll have a look.

[backbone87]

ah we pushed at the same time

fwiw here is the fixed I prepared locally:

    const connection = new OperatorSubscriber<T>(subscriber, (value) => {
      if (++seen === count) {
        // unsubscribe from the source if we have taken enough after the current
        // value. needs to be done before notifying downstream, so we dont get
        // any more notifications from upstream through sync recursion
        connection.unsubscribe();
        subscriber.next(value);
        subscriber.complete();
      } else if (seen < count) {
        subscriber.next(value);
      }
    });

    source.subscribe(connection);

though this causes a whitebox test for delayWhen to fail

[cartant]

Probably want to remove the breaking change label from the description if this is gonna be merged as just a test.

Also, the reason I deem this a bug is because the operator is knowingly letting stuff - side effects - happen when it could immediately unsubscribe.

And, FWIW, the fix I was gonna make was basically what's ☝

[backbone87]

feel free to edit around the PR & branch, I wont push anymore now to avoid any more conflicts since @benlesh is working on it

[benlesh]

@cartant yeah, we're already handling reentrancy in the next case, just not in the error and complete cases. This fix is simple enough.

[benlesh]

Thanks for this, @backbone87, very helpful!

[cartant]

Now that you're a contributor, @backbone87, I don't have to approve CI runs on your PRs. 🎉

[benlesh]

Follow up PR is here: #6396