Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(IteratorObservable): Observables from generators will now final… #2038

Merged
merged 1 commit into from
Oct 15, 2016
Merged

fix(IteratorObservable): Observables from generators will now final… #2038

merged 1 commit into from
Oct 15, 2016

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Oct 14, 2016

...ize when subscription ends

In order to model the behavior of for..of when consuming a Generator, if a break is hit in the for..of, return() is called on the generator and the generator will jump to a finally block if it has one. Observables created from generators will now have this same behavior.

    Observable.from((function* () {
      try {
        yield 1;
        yield 2;
        yield 3;
      } finally {
        console.log('finalized');
      }
    })())
    .take(2)
    .subscribe(x => console.log(x));

    // should log
    // 1
    // 2
    // finalized

fixes #1938

…ize when subscription ends

In order to model the behavior of `for..of` when consuming a Generator, if a `break` is hit in the `for..of`, `return()` is called on the generator and the generator will jump to a `finally` block if it has one. Observables created from generators will now have this same behavior.
```js
Observable.from((function* () {
  try {
    yield 1;
    yield 2;
    yield 3;
  } finally {
    console.log('finalized');
  }
})())
.take(2)
.subscribe(x => console.log(x));

// should log
// 1
// 2
// finalized
```

fixes #1938
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 97.041% when pulling 22d286a on blesh:close_iterator into 7ee0575 on ReactiveX:master.

@jayphelps
Copy link
Member

Anyone else reading this later, keep in mind that generators like this are basically singletons, so if one subscribers forces the generator to .return(), then every subscriber is forced into that.

const stream$ = Observable.from((function* () {
  try {
    yield 1;
    yield 2;
    yield 3;
  } finally {
    console.log('finalized');
  }
})());

stream$.take(1).subscribe();
stream$.take(3).subscribe();
// Second subscriber will only get one, not three because
// first subscriber is an scumbag

@jayphelps
Copy link
Member

jayphelps commented Oct 14, 2016

looks good, will wait a few for others to chime in since this is one of those "one person's bugfix is another person's breaking change" so shipping this at RC might be mean--but let's be honest..how many people are using generators with Rx...

@benlesh
Copy link
Member Author

benlesh commented Oct 14, 2016

Related is the issue I've created in the TC39 spec. We might even want to wait for a ruling there... tc39/proposal-observable#115

@benlesh
Copy link
Member Author

benlesh commented Oct 15, 2016

Given that this updates RxJS 5 to match the direction of the observable spec, I think it's the proper path. tc39/proposal-observable#115

@jayphelps
Copy link
Member

LGTM

@jayphelps jayphelps merged commit 3666cbc into ReactiveX:master Oct 15, 2016
@lock
Copy link

lock bot commented Jun 6, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rx.Observable.from..take does not close iterator
3 participants