bergus / promise-cancellation

Promise cancellation proposal for JavaScript
Other
24 stars 1 forks source link

Our implementation #12

Closed cyrilchapon closed 6 years ago

cyrilchapon commented 6 years ago

Here's our implementation, slightly different from the proposal, but working.

https://github.com/permettez-moi-de-construire/cancellable-promise

const {
  cancellablePromise,
  CancelError,
  CancelToken
} = require('@permettezmoideconstruire/cancellable-promise')

// Wrap a promise
const initialPromise = SOMETHING_ASYNC()
const cancelToken = new CancelToken()
const wrappedPromise = cancellablePromise(initialPromise, cancelToken)

// Somewhere, cancel the promise...
cancelToken.cancel()

//Then catch it
wrappedPromise
.then((res) => {
  //Actual, usual fulfill
})
.catch((err) => {
  if(err instanceOf CancelError) {
    //Handle cancel error
  }

  //Handle actual, usual error
})
bergus commented 6 years ago

Hi! How exactly is it different? I didn't find any documentation in your repo. Can you maybe add an example for how to cancel a then callback (the most important feature)?

cyrilchapon commented 6 years ago

You can cancel any promise you want, including a then-returned one.

the basic scenario

const {
  cancellablePromise,
  CancelError,
  CancelToken
} = require('@permettezmoideconstruire/cancellable-promise')

// Wrap a promise
const initialPromise = SOMETHING_ASYNC()
const cancelToken = new CancelToken()
const wrappedPromise = cancellablePromise(initialPromise, cancelToken)

// Somewhere, cancel the promise...
cancelToken.cancel()

//Then catch it
wrappedPromise
.then((res) => {
  //<-- THIS WILL NOT BE CALLED IF CANCEL OCCUR BEFORE FULFILL
})
.catch((err) => {
  if(err instanceOf CancelError) {
    //Handle cancel error
  }

  //Handle actual, usual error
})

Is about cancelling the root promise, but you can imagine more complex stuff, like

const {
  cancellablePromise,
  CancelError,
  CancelToken
} = require('@permettezmoideconstruire/cancellable-promise')

// Wrap a promise
const initialPromise = SOMETHING_ASYNC()

const cancelToken1 = new CancelToken()
const cancelToken2 = new CancelToken()

const wrappedPromise = cancellablePromise(initialPromise, cancelToken)

//Catch it
wrappedPromise
.then((res) => cancellablePromise(
  MakeAnythingElseAsync(res)
))
.then((res) => {
  //Final result
})
.catch((err) => {
  if(err instanceOf CancelError) {
    //Handle cancel error
  }

  //Handle actual, usual error
})

this is just an example. As this is very composable, you can imagine what you want.

It just basically wraps any promise, and make it reject if cancel() if called is called before fulfill.

Two promises can even share the same token. Two promises of a same chain can have different tokens.

There is no further documentation, because this is not further complicated 😄

You can read tests, though.

This is pretty young repo, but anything is welcome :)

EDIT: just realized your point, about "cancelling the then". Well this is actually pointless, I guess, since in real world there are 2 and only 2 scenarios :

This implementation rejects with CancelError, if and only if, cancel occurs before fulfill / rejection, which I believe should be natural behavior

bergus commented 6 years ago

The problem with

const initialPromise = Promise.resolve(somValue)
const cancelToken = new CancelToken()
const wrappedPromise = cancellablePromise(initialPromise, cancelToken)
cancelToken.cancel() // Somewhere, cancel the promise...
wrappedPromise.then((res) => {
  //<-- THIS WILL NOT BE CALLED IF CANCEL OCCUR BEFORE FULFILL
})

is that there's an edge case where the callback is still called even when the token was already cancelled. This inconsistency is a horrible potential source of bugs, even if (or especially when) it rarely happens in the real world, because nobody thinks about it.

Also I really dislike the syntax of having to write

cancellablePromise(initialPromise, cancelToken)
.then(res => cancellablePromise(
  MakeAnythingElseAsync(res), cancelToken
))
.then(res => {
  // Final result
})

all the time. The syntax in this proposal is much simpler:

initialPromise
.then(MakeAnythingElseAsync, cancelToken)
.then(res => {
  // Final result
}, cancelToken)

while working mostly the same.

This implementation rejects with CancelError, if and only if, cancel occurs before fulfill / rejection, which I believe should be natural behavior

Yes, I share that belief, but with the additional requirement that it should not cause an unhandled rejection.

cyrilchapon commented 6 years ago

there's an edge case where the callback is still called even when the token was already cancelled

Can you elaborate a little on this ? Any reference or something ? I'd be curious how to improve our library.

having to write

cancellablePromise(initialPromise, cancelToken)
.then(res => cancellablePromise(MakeAnythingElseAsync(res), cancelToken)

instead of

initialPromise
.then(MakeAnythingElseAsync, cancelToken)

Well... your first promise is just not cancellable anymore.. and you have to..

Promise.resolve()
.then(initialPromise, cancelToken)

If you want to perform something specific on first promise cancellation. Which is actually much uglier, isn't it ? I believe having actually two promises is not the usual use-case, and "Making a promise cancellable" would be, maybe, a better approach of "making a .then cancellable". This is my humble opinion.

But I get your point straight, a signature improvement is probably much more like a v2 for promise than a simple wrapper of mine. This was intentional, temporary before anything official, thus doesn't realy conflict with your spec.

Yes, I share that belief, but with the additional requirement that it should not cause an unhandled rejection.

Why would it generate an unhandled rejection ? It generates a rejection, handling it is up to the user.

bergus commented 6 years ago

Can you elaborate a little on the edge case?

See https://github.com/bergus/promise-cancellation/blob/master/trifurcation.md. The example given there might be pathological, but still I think this is an important issue.

your first promise is just not cancellable anymore..

The initialPromise in the example never was cancellable, and cannot be made to be (you cannot alter a promise from outside). Your approach is to create a wrappedPromise (using the token) and installing the then callback on that. My approach was to pass the token right into the then call next to the callback, which has the same end result: it won't call the callback when the token was cancelled, and it returns a ("cancellable") promise that gets rejected when the cancellation is requested on the token.

You have to Promise.resolve().then(initialPromise, cancelToken) if you want to perform something specific on first promise cancellation.

I don't understand what you mean, you cannot pass a promise to then as a callback. Also the initialPromise is never cancelled by our cancelToken, so there's nothing to observe. If we want to perform an action when the cancellation happens, we just subscribe to the token.

Why would it generate an unhandled rejection? It generates a rejection, handling it is up to the user.

Because a cancellation means that I want to ignore the result and don't care about the operation any more. That includes no longer wanting to handle any errors from it. Especially I would not want to need to handle rejections caused by the cancellation I knowingly just performed.

a signature improvement like in your spec is probably much more like a v2 for promise

Yes. But I fear my proposal is dead and nothing official will come, so we're left with simple wrappers. If you want to support my proposal, or voice your disagreement with the things I wrote in the documents in this repo, you're always welcome of course :-)

cyrilchapon commented 6 years ago

Some great stuff in every points here.

That sad this discussion seems closed in JS ecosystem.

Thanks for pointing out the edge case. I need more tests in my repo, but I did include an if "is already cancelled" or something when rejection / fulfill happen (which, is actually, way more simple to perform with the wrapper approach)

I personally (but that's my humble opinion) think that a cancellation is a kind of rejection. I could elaborate on this, but a main argument would be to check Bluebird and Axios takes on cancellation. Though, indeed handling both then and catch in a different way, or just ignore both of them, on cancellation makes sense..