Cancellable forEach #3977
Cancellable forEach #3977
Conversation
- Adds documentation - Deprecates promise constructor
src/internal/Observable.ts
Outdated
| forEach(next: (value: T) => void, promiseCtor?: PromiseConstructorLike): Promise<void> { | ||
| forEach(next: (value: T) => void, promiseCtorOrSubscription?: PromiseConstructorLike|Subscription): Promise<void> { | ||
| let promiseCtor: PromiseConstructorLike; | ||
| let subs = isSubscription(promiseCtorOrSubscription) ? promiseCtorOrSubscription : undefined; | ||
| promiseCtor = getPromiseCtor(promiseCtor); |
There was a problem hiding this comment.
Shouldn't promiseCtorOrSubscription be passed here? In the refactoring, the promiseCtor that's passed to getPromiseCtor will always be undefined.
spec/Observable-spec.ts
Outdated
| // Since the consuming code can no longer interfere with the synchronous | ||
| // producer, the remaining results are nexted. | ||
| expect(results).to.deep.equal([1, 2, 3, 4, expected]); | ||
| expect(results).to.deep.equal([1, 2, 3, expected]); |
There was a problem hiding this comment.
The change to this test's expectation suggests that the behaviour of the previous, non-cancellable implementation was actually a bug - as next was called despite an error being effected. The new behaviour makes more sense to me, as an error effected from next sees the observer unsubscribe.
Could the comment be removed? It's no longer applicable, as the remaining results are not nexted.
There was a problem hiding this comment.
Yup. I'll make sure to add the fix to the changelog too.
…by passed Subscription
|
@cartant ... I'm having it reject with an |
|
What I am worried about is forward compatibility with whatever cancellation primitive makes it into the spec. We already have AbortSignal in the DOM spec, and there is ongoing work to get a primitive into ES, which I would love love love to see supported seamlessly in rxjs. |
I'm not worried about this in the slightest. We can deprecate and pivot if we need to, but honestly, I have yet to see a proposal or cancellation type that isn't compatible with what we have here. The committees are slow, and unlikely to pass anything related to any of this. CancellationToken failed once. Observable is stuck at stage 1 forever. |
forEach, by allowing aSubscriptionto be passed as a sort of cancellation token.forEach. (That can be configured globally, and I'd like to remove it for v7)Behavior Described
You can read the docs in the PR for this, but basically:
If you call
forEachwith a second argument that is aSubscription, that subscription will act as a cancellation token, such that if youunsubscribefrom it, the forEach's returned promise will reject with anErrorwith the messageObservable forEach unsubscribed. This is done, so there's a way to gracefully handle cancellations in async-await code, and I think provides a better alternative totakeUntilin those use cases.