fix(take): deal with non-integers

[backbone87]

Description:

make take deal with non-integer counts.

i presented two possibilities how to deal with them:

1. POSITIVE_INFINITY -> identity; NaN -> error; count >= MAX_SAFE_INTEGER + 1 -> error
2. NaN -> empty; count >= MAX_SAFE_INTEGER + 1 (includes POSITIVE_INFINITY) -> identity

BREAKING CHANGE:

• NaN was previously handled as POSITIVE_INFINITY (could be considered a bug) actually it was treated as ignoreElements since all next notifications were just blocked
• only for 1.: count > MAX_SAFE_INTEGER + 1 was previously handled as POSITIVE_INFINITY, since we cant increment seen by 1 beyond MAX_SAFE_INTEGER + 1

i will add test cases if the approach is agreed upon

[josepot]

nit-pick:

Suggested change

	if (!(count < Number.MAX_SAFE_INTEGER + 1)) {
	if (!(count <= Number.MAX_SAFE_INTEGER)) {

(one byte lighter 🙈 )

[backbone87]

actually, this was done intentionally, since Number.MAX_SAFE_INTEGER + somethingSmallerThanOne is ok, (gets floored after). but it could very well be that there is no number representable between MAX_SAFE_INTEGER and MAX_SAFE_INTEGER + 1 in 64bit floats

[benlesh]

Suggested change

	if (!(count < Number.MAX_SAFE_INTEGER + 1)) {
	if (Number.MAX_SAFE_INTEGER < count) {

1. Inverting the check makes it less readable.
2. Accounting for Number.MAX_SAFE_INTEGER + 0.56 or whatever won't make a difference, because you can't take 0.56 of something :)

[benlesh]

We have ArgumentOutOfRangeError, which could be used here.

However, a bigger issue is this makes this fix a breaking change.

[benlesh]

There are a few suggestions on here.

I'm not sure about throwing the error, though. Odds are, it's a non-issue. But if we do, we need to use our existing ArgumentOutOfRangeError, I think.

However, I am concerned about the breaking change here. We might not be able to do this until version 8.

In all honesty, the checking of MAX_SAFE_INTEGER in general feels like overkill, because of someone really wants to take 9,007,199,254,740,991 values... at 1 per millisecond, that's more than 200,000 years. Even at 1 per nanosecond, that's like 100 days. heh.

If anything, we should treat it like we do infinity. However, my gut tells me we can just ignore it entirely and we'll never see an issue filed around it.

[benlesh]

Suggested change

	if (count === Number.POSITIVE_INFINITY) {
	if (count === Infinity) {

[backbone87]

is there a preference for the global Infinity? its the same value as Number.POSITIVE_INFINITY, personally I prefer the latter

[backbone87]

So we go with the other behavior:

• count === NaN -> EMPTY
• count >= MAX_SAFE_INTEGER + 1 -> identity

is that ok?

i reverted the PR to previous behavior and added failing test cases

[benlesh]

@backbone87 I think we can just not worry about MAX_SAFE_INTEGER, for the time being. Right now, the worst thing that happens is the user gets pinned to MAX_SAFE_INTEGER. Because all < or > checks past there will return false, and the number is so absurdly big that it just seems like a waste to even worry about it. Also, I'm not sure anything written in stone about changing MAX_SAFE_INTEGER.. as silly as it sounds, maybe that number goes up at some point with a Runtime 20 years down the road when we all have the new super CPUs in our brain chips running Chromium 2,000,000. Infinity on the other hand, will still be Infinity.

return !count || count <= 0
  ? EMPTY
  : count === Infinity
  ? identity
  : ...

[backbone87]

i can remove the check for MAX_SAFE_INTEGER, but as a matter of fact, the behavior is still the same as infinity, just that we keep a counter that will be stuck at some point (that will never be reached ofc)

edit:
I just noticed that we need 2 checks anyway,

if (!(count >= 1)) { // handles NaN as well
  return () => EMPTY;
}
if (!(count < Number.MAX_SAFE_INTEGER + 1)) { // handles infinity as well
  return identity;
}

what I really mean here: we have 3 main behaviors (EMPTY, everything, counted), it doesn't matter what check we use

[backbone87]

i have pushed now a version, with non-erroring behavior and keeping BC for all "sane" outcomes.

changed behaviors (breaking or bugfix?):

• NaN -> 0 insteadof ignoreElements
• non-integers > 0 & < 1 -> 0 insteadof ignoreElements
• non-integers > 1 -> floor(count) insteadof "taking up to count and then turn into ignoreElements"

optimized handling (non-breaking):

• counts that can not be incremented up to because of resolution limitations of 64bit floats or infinity counts just return an identity operator function instead of keeping an eventually stuck counter

[backbone87]

this entire check is not needed. the behavior of operate below will be the same as identity if count >= Number.MAX_SAFE_INTEGER + 1 (including Infinity). we probably don't expect to hit this intentionally with high numbers, but using take(condition ? n : Infinity) could be a valid pattern. in any way, we don't need to count seen in these cases.

[benlesh]

Noted from Core Team discussion. We probably don't want to defend against MAX_SAFE_INTEGER, because it's so big, that it seems like it's bloat for no real benefit.

[backbone87]

will remove. just for confirmation: the infinity case should also not be handled in a special way? (the current MR implementation covers this in the check for MAX_SAFE_INTEGER)