Opened 6 years ago

Closed 5 years ago

#17460 closed enhancement (wontfix)

[patch][ccla]dojo/promise/all does not cancel contained promises

Reported by: Bill Reed Owned by: Kris Zyp
Priority: undecided Milestone: 2.0
Component: General Version: 1.9.1
Keywords: Cc: Mark Wubben, Adam Peller
Blocked By: Blocking:

Description

one of the key features of dojo/deferred is the ability to cancel outstanding deferred preventing the .then processing. dojo/promise/all is missing this key feature. With deferred I can pass a canceler function that is called when the promises is canceled, this function can return a error instance that can then be processed in the error function of the deferred as a canceled promises instead of a error. This is missing with promise/all and makes it impossible to determine it the promise/all was canceled or had an error.

I have attached a patch that would allow the user to pass an optional canceler function to the all(objectArray, canceler) Will be invoked if the deferred is canceled. The canceler receives the reason the deferred was canceled and the promises array as its arguments. If a canceler is not passed the default canceler function will be used which will cancel all the unfulfilled promises and return a array of error objects.

Attachments (4)

allTest.html (1.9 KB) - added by Bill Reed 6 years ago.
testCase
all.patch (2.4 KB) - added by Bill Reed 6 years ago.
version using array.map
backwardCompat_allTest .html (2.0 KB) - added by Bill Reed 6 years ago.
backward compatible all test case
backWardCompat_all.patch (2.3 KB) - added by Bill Reed 6 years ago.
backward compatible all patch

Download all attachments as: .zip

Change History (14)

comment:1 Changed 6 years ago by Bill Reed

I have attached a updated version of the patch as well as a test case to demonstrate the patch:

before the patch the test case output:

Not yet started. I'm running... I'm running... My process completed! I'm finished, and the result was: finished My process completed! I'm finished, and the result was: finished

after the patch is applied:

Not yet started. I'm running... I'm running... I was cancelled with reason: goodbye I was cancelled with reason: goodbye

Changed 6 years ago by Bill Reed

Attachment: allTest.html added

testCase

comment:2 Changed 6 years ago by Adam Peller

Cc: Mark Wubben Adam Peller added
Owner: set to Kris Zyp
Status: newassigned
Summary: dojo/promise/all does not cancel contained promises[patch][ccla]dojo/promise/all does not cancel contained promises
Type: defectenhancement

Changed 6 years ago by Bill Reed

Attachment: all.patch added

version using array.map

comment:3 Changed 6 years ago by ben hockey

this looks like it's not backwards compatible, right? previously the array of promises were not cancelled when the promise returned from all was cancelled - the description even said this explicitly, so it was intentional.

comment:4 Changed 6 years ago by Adam Peller

Good point. Perhaps it would be easiest to make this a 2.0 request?

comment:5 Changed 6 years ago by Adam Peller

Milestone: tbd2.0

comment:6 Changed 6 years ago by ben hockey

i don't know what others think for 2.0 but i'm wondering if we should be working towards using the native Promise API being proposed for ECMAScript and about to be shipped in chrome (https://code.google.com/p/chromium/issues/detail?id=295420), firefox (https://bugzilla.mozilla.org/show_bug.cgi?id=911216) and webkit (https://bugs.webkit.org/show_bug.cgi?id=120954)

that would make it an easy transition to leverage the native implementation as it becomes available.

comment:7 Changed 6 years ago by bill

If we do want to follow the native promises spec then we can just build on Alex's polyfill, specifically the every() implementation. As discussed on the mailing list, the downside of following the spec is that it's slow because every then() invocation calls setTimeout() or similar.

AFAICT from the spec, canceling a Promise returned from every() doesn't do anything to the underlying promises, in which case I guess this ticket should be closed as wontfix.

Changed 6 years ago by Bill Reed

backward compatible all test case

Changed 6 years ago by Bill Reed

Attachment: backWardCompat_all.patch added

backward compatible all patch

comment:8 Changed 5 years ago by dylan

Should we consider this for 1.11, or just be happy with the work in 2.0? https://github.com/dojo/core/blob/master/src/Promise.ts

comment:9 Changed 5 years ago by Mark Wubben

ES6 promises don't even define cancellation. At this point I'd steer clear of implementations that diverge too far from the spec.

So no, don't consider it, and I'd argue don't add cancellation to the new Promise implementation either.

comment:10 Changed 5 years ago by dylan

Resolution: wontfix
Status: assignedclosed

Per Mark's comment, we've simply provided a shim for the spec for promises with Dojo 2, so closing this one out.

Note: See TracTickets for help on using tickets.