Adding Promise.prototype.finally

[dfahlander]

This PR adds Promise.prototype.finally (as it is now part of ES2018).

Please test and verify it is functional before merging! I've added tests, so it should be fine to merge.

The code is mostly copied from https://github.com/matthew-andrews/Promise.prototype.finally/blob/master/finally.js

[dfahlander]

I've now added unit tests for Promise.prototype.finally as well, so I believe it should be safe to merge now.

[taylorhakes]

@dfahlander Thanks for doing this. It looks great! I added a few comments. After you make the requested changes, I will be happy to merge this in.

[taylorhakes]

Based on the spec and the implementation in Chrome, if the the callback is not callable (not a function), it should not throw an error. It should return a new Promise.

if (typeof callback !== 'function') {
  return Promise.resolve(value);
} 

[dfahlander]

Ah, nice you catched that ;) That's a change that should be updated in Promise.prototype.finally polyfill as well. I pretty much just copied that code. When thinking about it, I am also not sure that we need to do constructor.resolve() at all. We're already in a then- callback so we might be fine with just returning the result. Or am I wrong?

[taylorhakes]

I was suggesting putting that code before this.then, outside of the “then” to avoid 2 extra promises being created

[dfahlander]

I see. The thing I was thinking about was whether it would be safe to do:

Promise.prototype.finally = function (callback) {        
        if (typeof callback !== 'function') {
            return this.constructor.resolve(value);
        }
        return this.then(value => {
            callback(); // Not handling return value at all
            return value;
        }, err => {
            callback(); // Not handling return value at all
            return this.constructor.reject(err);
        });
}

But I see that it should not comply to the spec as it wouldn't await any promise returned by callback(), so let's forget about it...

[dfahlander]

Wait a sec. How could we put the typecheck before this.then() when we don't yet have a value?

Maybe this would do?

Promise.prototype['finally'] = function(callback) {
  if (typeof callback !== 'function') {
    return this; // Equal to this.then(value => value);
  }
  var constructor = this.constructor;
  return this.then(
    function(value) {
      return constructor.resolve(callback()).then(function() {
        return value;
      });
    },
    function(reason) {
      return constructor.resolve(callback()).then(function() {
        return constructor.reject(reason);
      });
    }
  );
};

[dfahlander]

To support ES3, I also put finally in brackets this time.

[taylorhakes]

This test is already covered in the below tests. I think it's OK to remove it.

[taylorhakes]

This assert is unnecessary. It never fails. Can you change this to

Promise.resolve(3).finally(function() {
  assert.equal(arguments.length, 0,  'No arguments to onFinally');
  done();
});

[dfahlander]

Actually it would fail if it was never called (by a timeout in the testing framework). But I can check there are no args provided. Should also add a new test verifying that if the handler returns a promise, it must be awaited for.

[taylorhakes]

This assert is unnecessary. It never fails. Can you change this to

Promise.reject(new Error()).finally(function() {
  assert.equal(arguments.length, 0,  'No arguments to onFinally');
  done();
});

[taylorhakes]

Looks good. Thanks for making the change

[taylorhakes]

Sorry, I made a mistake on this. This isn't enough to find all callable types. Here is the correct implementations https://github.com/npetruzzelli/es-abstract-is-callable/blob/master/lib/es2017-v8/IsCallable.js .

I don't think writing that much code is worth it (for the goal of this library being a small library). I think removing this if statement is a better idea. It will obviously throw an error if someone passes a non callable, but I prefer to support different types of callables.

Sorry to ask this to be changed back, I didn't fully understand the tradeoff.

[taylorhakes]

Everything looks great. One last comment about my mistake about callables.

[taylorhakes]

Thanks again for doing this. Really appreciate it!

[dfahlander]

Thanks for providing this minimal and precise promise library. Your code has been a great resource for me ever since 2014 when I wrote Dexie as I needed some minimal and easy-to-understand promise code that I could extend (needed to tweak the code to be indexedDB-compliant due to this issue. Your code has been a corner stone for Dexie is. So thanks back.

http://dexie.org/docs/Promise/Promise#implementation-details