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

Revert "Cancellable forEach" #3987

Merged
merged 1 commit into from
Aug 6, 2018
Merged

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Aug 3, 2018

Reverts #3977

Currently, there's an issue with what is in there if you unsubscribe from a synchronous observable, synchronously, but after the observable completes:

const subs = new Subscription();

of('test').forEach(x => console.log(x), subs)
  .then(() => console.log('this should not be called'));

subs.unsubscribe();

// Logs
// "test"
// "this should not be called"

Solutions around this aren't too bad, but it adds a lot of bloat, mostly due to the fact we're accepting PromiseConstructorLike as an argument, and we're beholden to use it.

I want to revert this new feature until:

  1. We deprecate and remove all Promise Constructor configuration (This is legacy, and users should use a proper polyfill)
  2. We deprecate and update the behavior of forEach to call it's next handler on the next microtask, as per the proposals. (We can't do this until v7 because it's a breaking change)

I still want to use Subscriptions as a cancellation here, but it's going to have to wait.

Sorry folks.

@benlesh
Copy link
Member Author

benlesh commented Aug 3, 2018

NOTE: We'll still need to redo the fix to forEach to make sure additional emissions aren't still happening after an error is thrown.

@rxjs-bot
Copy link

rxjs-bot commented Aug 3, 2018

Warnings
⚠️

commit message does not follows conventional change log (1)

(1) : RxJS uses conventional change log to generate changelog automatically. It seems some of commit messages are not following those, please check contributing guideline and update commit messages.

Generated by 🚫 dangerJS

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.005%) to 96.947% when pulling f882647 on revert-3977-cancellable-forEach into bb77e25 on master.

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disappointing that the reversion is necessary, but LGTM.

@benlesh benlesh merged commit 1f056f5 into master Aug 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Sep 5, 2018
@benlesh benlesh deleted the revert-3977-cancellable-forEach branch January 23, 2019 17:45
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.

4 participants