fix(IteratorObservable): get new iterator for each subscription

[kwonoj]

Description:
Our IteratorObservable gets an iterator from the object once only when it's constructed, so any subsequent subscription is not able to iterate since the first subscription will make iterator completes.

This PR updates behavior to get corresponding new iterator per each subscription, allows any subscription can correctly iterate object. I'm labeling this for next patch version, since I believe this isn't breaking changes.

Related issue (if exists):

• closes Second subscription to the same observable is not starting a sequence if the observable was built from a typed array #2496

[coveralls]

Coverage remained the same at 97.689% when pulling 00f963b on kwonoj:fix-iter into 3ced24f on ReactiveX:master.

[benlesh]

This definitely a bug fix. The issue is that it's going to be a breaking change for somebody. So we might want to put this in the next major (which we should publish soon). But it's debatable.

The problem, better outlined is this:

const data = new Int32Array([1, 2, 3, 4, 5]);

const source$ = Observable.from(data);

source$.toArray().subscribe(x => console.log(x)); // [1, 2, 3, 4, 5]

source$.toArray().subscribe(x => console.log(x)); // []

That's not what users would expect at all. It should behave like the standard Array implementation. However, we might have one crazy person relying on this behavior.

The work around to get that behavior again would be to pass the actual iterator itself:

const data = new Int32Array([1, 2, 3, 4, 5]);
const iterator = data[Symbol.iterator]();

// "new" appropriate behavior
const source1$ = Observable.from(data);
source1$.toArray().subscribe(x => console.log(x)); // [1, 2, 3, 4, 5]
source1$.toArray().subscribe(x => console.log(x)); // [1, 2, 3, 4, 5]

// "old" behavior, if desired
const source2$ = Observable.from(iterator);
source2$.toArray().subscribe(x => console.log(x)); // [1, 2, 3, 4, 5]
source2$.toArray().subscribe(x => console.log(x)); // []

[staltz]

The issue is that it's going to be a breaking change for somebody.

Pros of major: no one will be surprised, no code will break in production. Cons: it will be a different number. Pros of patch: it will look like a harmless corner-casey number bump, like this is a corner case. Cons of patch: there is a positive probability someone will be surprised, and that someone could be a user of an app in production.

I vote for major.

Major version X (X.y.z | X > 0) MUST be incremented if any backwards incompatible changes are introduced to the public API.

http://semver.org/#spec-item-8

It depends what we consider our public API. Usually its tests + docs. We don't have any test that asserts the buggy behavior as normal. In our docs we say

Creates an Observable from an Array, an array-like object, a Promise, an iterable object, or an Observable-like object.

It could be understood that the Iterator creation side effect happens once, independent of multiple subscriptions, like the promise constructor side effect does as well.

[jayphelps]

I think this is a major bump for the reasons outline by Andre.

[kwonoj]

Removing labels for patch to avoid accidental check in.

[benlesh]

@kwonoj can you resolve conflicts against next please?

[kwonoj]

sure.

[kwonoj]

@benlesh rebased to next.

[coveralls]

Coverage remained the same at 97.737% when pulling 1ea94c2 on kwonoj:fix-iter into 3003614 on ReactiveX:next.

[rxjs-bot]

	Messages
📖

CJS: 1346.4KB, global: 746.4KB (gzipped: 120.1KB), min: 145.7KB (gzipped: 31.2KB)

Generated by 🚫 dangerJS

[kwonoj]

Now this can go into master as breaking change.

[coveralls]

Coverage decreased (-0.01%) to 97.359% when pulling bbc3073 on kwonoj:fix-iter into 040d951 on ReactiveX:master.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.