Add `.cancel()` #17

timdp · 2017-02-05T12:07:16Z

As suggested in #15.

Notes:

This depends on ES2015ify #16.
Currently, abort() throws an AbortError. Perhaps that behavior should be optional? I usually find myself wanting to cancel everything that follows, and I'd rather not add a catch in those cases.
I'm introducing a dependency on p-defer, which is lightweight, but still relevant.
I'm not entirely sure which issue the unhandled rejection handler originally solved, so the PR probably introduces an issue with that. I'm not getting any warnings from the tests though. Is it the known failure in the tests?

timdp · 2017-02-05T19:33:59Z

Thanks for merging #16. 👍 I've rebased this one.

SamVerschueren

Nice work @timdp !

SamVerschueren · 2017-02-05T19:38:28Z

index.js

+
+class AbortError extends Error {
+	constructor(message) { // eslint-disable-line no-useless-constructor
+		super(message);


this.name = 'AbortError';

https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/docs/rules/custom-error-definition.md

I believe you can remove the // eslint-disable-line no-useless-constructor as well now.

Done!

I'd briefly pushed a version that included multipipe as a dependency because I accidentally npm-installed that in the wrong terminal tab. Make sure you don't accidentally merge that one. Sorry about that.

SamVerschueren · 2017-02-05T19:38:45Z

index.js

@@ -1,34 +1,48 @@
 'use strict';

+const defer = require('p-defer');


Remove empty line above this statement

timdp · 2017-02-05T19:45:00Z

Thanks!

timdp · 2017-02-18T16:44:25Z

Just fixed a typo in the readme. The code hasn't changed.

sindresorhus · 2017-02-18T18:13:15Z

Currently, abort() throws an AbortError. Perhaps that behavior should be optional? I usually find myself wanting to cancel everything that follows, and I'd rather not add a catch in those cases.

What should happen in that case though? Never resolving the promise? That would mean any promise after would never resolve. Resolve with no value? Promises after might depend on some value that are to be passed through.

I'm not entirely sure which issue the unhandled rejection handler originally solved, so the PR probably introduces an issue with that. I'm not getting any warnings from the tests though. Is it the known failure in the tests?

Me neither

sindresorhus · 2017-02-18T18:15:35Z

I wonder, now that the official promise cancelation is dead (Meaning, we don't risk being incompatible with the offical spec), should we use .cancel() for consistency with Bluebird?

timdp · 2017-02-18T18:56:42Z

Yeah, I'm not saying an ever-pending promise is a great solution. I just wanted to mention the common case of wanting to cancel with the intention of not running continuations.

I'm more than happy to change abort to cancel. One little issue that it raises is whether the error should be a CancelationError (US English) or a CancellationError (UK English).

sindresorhus · 2017-02-19T07:27:07Z

Yeah, I'm not saying an ever-pending promise is a great solution. I just wanted to mention the common case of wanting to cancel with the intention of not running continuations.

I was really asking. What do you think should happen? I'm open to suggestion for solving that use-case.

I'm more than happy to change abort to cancel. One little issue that it raises is whether the error should be a CancelationError (US English) or a CancellationError (UK English).

Why not just CancelError? If we went with the other it should be CancelationError.

timdp · 2017-02-19T09:05:56Z

I was really asking. What do you think should happen? I'm open to suggestion for solving that use-case.

That's the thing, I don't know. :-) I suppose there's no cleaner way than a catch with an instanceof to swallow the error.

Why not just CancelError? If we went with the other it should be CancelationError.

Bluebird calls it CancellationError so one of my packages also does that for the sake of consistency. It's always slightly annoyed me, even though most Americans I know also seem to throw some British influences into their English as well.

Personally, I'm not a fan of CancelError because I think you'd be less likely to call it that in spoken or written English. I'd go so far as to say AbortError should be AbortionError, but there are obvious reasons not to call it that.

As a reference, Uber has something called a cancellation fee. Interestingly, they also use double L, while their language selector explicitly distinguishes between US English and UK English. Could be laziness on their part though.

SamVerschueren · 2017-02-19T12:15:02Z

In my opinion, it feels like when cancelling a promise, nothing that comes after that step should be executed. So resolving with an empty value doesn't feel right.

So I'm in doubt between throwing something or just do nothing. And to be honest, I'm more in favour of doing nothing because, as the method implies, it is aborted/cancelled. To me it doesn't feel right that still some code inside the chain is being excited, not even with a specific error object because it was cancelled entirely. It's just my opinion :).

The thing I wonder though when you do that, does that mean we have a dangling promise that can not be cleaned up by the GC? Because it's in an idle state (neither resolved or rejected)? I don't think so, but it might.

timdp · 2017-02-19T12:22:19Z

I assume it'll be problematic for GC as long if the promise remembers its .then() and .catch() handlers internally, so unless we intercept those and clean them up ourselves, it's probably going to be the case, right?

An alternative could be to always resolve and have a .canceled (or .cancelled 🤷‍♂️) property that you can inspect in the .then(). However, that's bound to cause issues, and it doesn't make a lot of sense from a semantical perspective.

Since this is never going to get standardized, I think the closest we can get is throwing an error.

timdp · 2017-02-19T12:32:12Z

I just ran this basic memory benchmark on Node 6:

const run = () => {
  global.gc()
  console.log('before:', process.memoryUsage().heapUsed)
  for (let i = 0; i < 1000000; ++i) {
    new Promise((resolve, reject) => {}).then(() => {})
  }
  setTimeout(() => {
    global.gc()
    console.log('after: ', process.memoryUsage().heapUsed)
    setTimeout(run, 5000)
  }, 5000)
}

run()

First 3 iterations on my machine:

before: 4019928
after:  4001456
before: 4005280
after:  3852776
before: 3731160
after:  3750688

So actually, it wouldn't be an issue in terms of performance.

sindresorhus · 2017-03-20T05:40:38Z

I'd be open to an option to just resolve with nothing instead of throwing, but I don't want it as the default behavior and it should be a separate pull request. We can't do nothing, for example, await delay() with doing nothing, would hang forever. But anyways, let's discuss this in a separate issue/PR.

sindresorhus · 2017-03-20T05:49:22Z

Let's go with .cancel() and CancelError. I'm already using CancelError in other modules and I'm not really concerned about compatibility with Bluebird as it has different semantics anyways, and none of this is standardized.

Why CancelError? I personally find that clearer. And in the Promise cancel spec they introduced a new type Cancel and this is inspired by that. Also makes it less ambiguous whether it's CancellationError or CancelationError. And last, in JS, you have for example EvalError, not EvaluationError.

timdp · 2017-03-21T12:51:20Z

Okay, PR updated. Branch name doesn't make sense anymore though.

sindresorhus · 2017-03-22T10:50:15Z

Awesome. Thanks for your patience @timdp :)

timdp · 2017-03-22T11:00:52Z

No problem. I prefer to get things right. :) Thanks for merging!

cheton · 2017-03-27T03:01:31Z

I encountered an issue after upgrading to delay@2.0 when running a webpack build with UglifyJS:

https://travis-ci.org/cncjs/cncjs/jobs/215250557#L7209

Do you have any clues on that? Thanks.

sindresorhus · 2017-03-27T03:29:27Z

@cheton Uglify doesn't support ES2015 syntax. See sindresorhus/ama#446.

SamVerschueren suggested changes Feb 5, 2017

View reviewed changes

SamVerschueren approved these changes Feb 5, 2017

View reviewed changes

sindresorhus changed the title ~~Add .abort()~~ Add .cancel() Mar 20, 2017

Add .cancel() to clear timeout

e538dee

sindresorhus merged commit 90d16f2 into sindresorhus:master Mar 22, 2017

sindresorhus mentioned this pull request Mar 22, 2017

Cancelation? #15

Closed

Repository owner locked and limited conversation to collaborators Mar 27, 2017

timdp deleted the abort branch April 14, 2017 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `.cancel()` #17

Add `.cancel()` #17

timdp commented Feb 5, 2017

timdp commented Feb 5, 2017

SamVerschueren left a comment

SamVerschueren Feb 5, 2017

SamVerschueren Feb 5, 2017

timdp Feb 5, 2017

SamVerschueren Feb 5, 2017

timdp commented Feb 5, 2017

timdp commented Feb 18, 2017

sindresorhus commented Feb 18, 2017 •

edited

Loading

sindresorhus commented Feb 18, 2017

timdp commented Feb 18, 2017

sindresorhus commented Feb 19, 2017

timdp commented Feb 19, 2017

SamVerschueren commented Feb 19, 2017

timdp commented Feb 19, 2017

timdp commented Feb 19, 2017

sindresorhus commented Mar 20, 2017

sindresorhus commented Mar 20, 2017 •

edited

Loading

timdp commented Mar 21, 2017

sindresorhus commented Mar 22, 2017

timdp commented Mar 22, 2017

cheton commented Mar 27, 2017

sindresorhus commented Mar 27, 2017

		@@ -1,34 +1,48 @@
		'use strict';

		const defer = require('p-defer');

Add .cancel() #17

Add .cancel() #17

Conversation

timdp commented Feb 5, 2017

timdp commented Feb 5, 2017

SamVerschueren left a comment

Choose a reason for hiding this comment

SamVerschueren Feb 5, 2017

Choose a reason for hiding this comment

SamVerschueren Feb 5, 2017

Choose a reason for hiding this comment

timdp Feb 5, 2017

Choose a reason for hiding this comment

SamVerschueren Feb 5, 2017

Choose a reason for hiding this comment

timdp commented Feb 5, 2017

timdp commented Feb 18, 2017

sindresorhus commented Feb 18, 2017 • edited Loading

sindresorhus commented Feb 18, 2017

timdp commented Feb 18, 2017

sindresorhus commented Feb 19, 2017

timdp commented Feb 19, 2017

SamVerschueren commented Feb 19, 2017

timdp commented Feb 19, 2017

timdp commented Feb 19, 2017

sindresorhus commented Mar 20, 2017

sindresorhus commented Mar 20, 2017 • edited Loading

timdp commented Mar 21, 2017

sindresorhus commented Mar 22, 2017

timdp commented Mar 22, 2017

cheton commented Mar 27, 2017

sindresorhus commented Mar 27, 2017

Add `.cancel()` #17

Add `.cancel()` #17

sindresorhus commented Feb 18, 2017 •

edited

Loading

sindresorhus commented Mar 20, 2017 •

edited

Loading