stefanpenner / es6-promise

A polyfill for ES6-style Promises
MIT License
7.29k stars 594 forks source link

include proposed `Promise.prototype.finally` #232

Closed stefanpenner closed 6 years ago

stefanpenner commented 8 years ago

basically just:

/**
  `finally` will be invoked regardless of the promise's fate just as native
  try/catch/finally behaves
  Synchronous example:

  findAuthor() {
    if (Math.random() > 0.5) {
      throw new Error();
    }
    return new Author();
  }
  try {
    return findAuthor(); // succeed or fail
  } catch(error) {
    return findOtherAuther();
  } finally {
    // always runs
    // doesn't affect the return value
  }

  Asynchronous example:

  findAuthor().catch(function(reason){
    return findOtherAuther();
  }).finally(function(){
    // author was either found, or not
  });

  @method finally
  @param {Function} callback
  @return {Promise}
*/
  finally(callback) {
    let promise = this;
    let constructor = promise.constructor;

    return promise.then(value => constructor.resolve(callback()).then(() => value),
                  reason => constructor.resolve(callback()).then(() => { throw reason; }));
  }

Questions:

domenic commented 8 years ago

/cc @ljharb

I am really unsure about whether you should wait longer. I don't have a lot of confidence in the current spec until https://github.com/tc39/proposal-promise-finally/issues/7 is done and a series of extensive tests are written. But at a glance those semantics are probably what we intend.

I guess I would hold off on including this in es6-promise until stage 3 or stage 4. But I dunno, this whole repo's name is a lie (promises are part of every ES, not just ES6, and have seen fixes since ES6; finally is not part of ES6), so it's not clear what the bar is for inclusion.

stefanpenner commented 8 years ago

@domenic ya, may need a rename as es-promise...

ljharb commented 8 years ago

While I don't expect any changes, I think waiting for stage 3 would be appropriate.

stefanpenner commented 8 years ago

@ljharb I'll keep it in a branch, and :shipit: when we hit stage 3..

elodszopos commented 8 years ago

Looking forward to this one boys!

shirakaba commented 6 years ago

Warning: Due to incorporating a future spec feature, this pull request breaks compatibility with the global Promise interface in all of the TypeScript core libraries (lib.es5.d.ts, lib.es6.d.ts, lib.es7.d.ts, and lib.es2015.promise.d.ts).

Any time one tries to use a Promise-returning core library function, such as navigator.requestMediaKeySystemAccess(), that function will return a Promise following the core library's typings. All versions after this pull request (eg. all versions after es6-promise v4.1.1) will no longer have compatible Promise typings with those of the core library, so the following error will appear:

Error:(12, 5) TS2719: Type 'Promise<T>' is not assignable to type 'Promise<T>'. Two different types with this name exist, but they are unrelated.
  Property 'finally' is missing in type 'Promise<T>'.

Could be my skills are lacking, but for now I'm unsure how to circumvent the error other than by pinning my es6-promise version to v4.1.1, or renaming it upon import to something other than 'Promise' – however, that defeats the purpose of a global polyfill.

ljharb commented 6 years ago

@shirakaba this isn't a future spec feature; finally is stage 4 - it's already in the language (in ES2018).

In other words, if the core TS library lacks a definition for finally, that's a bug in TS.

shirakaba commented 6 years ago

Okay, it looks like it may potentially even be a WebStorm IDE issue – Typescript has a lib called lib.esnext.promise.d.ts – importable via --lib esnext.promise, but WebStorm's language service does not have this library file, so I suspect I'd be unable to use it without issue.

WebStorm's language service has most of the TypeScript core libs, but also lacks lib.es2018.d.ts. Their lib.esnext.full.d.ts, like TypeScript's official one, does not include finally(), either. I'll have to look into it.

shirakaba commented 6 years ago

I have now tried adding esnext.promise.d.ts as a standalone lib to my project, yet its typings are not quite the same as ES6-promise's:

lib.esnext.promise.d.ts:

interface Promise<T> {
    /**
     * Attaches a callback that is invoked when the Promise is settled (fulfilled or rejected). The
     * resolved value cannot be modified from the callback.
     * @param onfinally The callback to execute when the Promise is settled (fulfilled or rejected).
     * @returns A Promise for the completion of the callback.
     */
    finally(onfinally?: (() => void) | undefined | null): Promise<T>
}

es6-promise.d.ts (based on v4.2.4 typings):

export interface Thenable <R> {
  then <U> (onFulfilled?: (value: R) => U | Thenable<U>, onRejected?: (error: any) => U | Thenable<U>): Thenable<U>;
  then <U> (onFulfilled?: (value: R) => U | Thenable<U>, onRejected?: (error: any) => void): Thenable<U>;
}

export class Promise <R> implements Thenable <R> {
// ...
    finally <U> (onFinally?: (callback: any) => U | Thenable<U>): Promise<U>;
// ...
}

I am now receiving a warning that the typings for finally() are mutually incompatible.

Impressively, I can get it to seemingly work by merging the two interfaces a bit:

lib.merged.d.ts:

interface Thenable <R> {
    then <U> (onFulfilled?: (value: R) => U | Thenable<U>, onRejected?: (error: any) => U | Thenable<U>): Thenable<U>;
    then <U> (onFulfilled?: (value: R) => U | Thenable<U>, onRejected?: (error: any) => void): Thenable<U>;
}

/**
 * Represents the completion of an asynchronous operation
 */
interface Promise<T> {
    /**
     * Attaches a callback that is invoked when the Promise is settled (fulfilled or rejected). The
     * resolved value cannot be modified from the callback.
     * @param onfinally The callback to execute when the Promise is settled (fulfilled or rejected).
     * @returns A Promise for the completion of the callback.
     */
    finally(onfinally?: (() => void) | undefined | null): Promise<T>
    finally <U> (onFinally?: (callback: any) => U | Thenable<U>): Promise<U>;
}

... But who knows what I may have broken downstream..?

shirakaba commented 6 years ago

Some options, both kinda bad:

Using es6-promise typings alongside core library's typings

tsconfig.json:

{
    "compilerOptions": {
        "lib": ["DOM","ES5","ScriptHost"]
    }
}

test.ts:

// Polyfills es6-promise to the global object.
require('es6-promise').polyfill();
// Gets typings and implementation from "es6-promise" module.
import {Promise} from "es6-promise";

// Works, due to using typings from es6-promise
Promise.race([]).finally();
// Works, due to using typings from core library
navigator.requestMediaKeySystemAccess();

navigator.requestMediaKeySystemAccess()
.then(() => {
    // Fails, due to mixing core-library promise chain with
    // a 'es6-promise' class function.
    Promise.resolve();
})
.catch((e: any) => {})
// Fails: would need to add the "esnext.promise" lib.
.finally();

Relying purely on the core library's typings

tsconfig.json:

{
    "compilerOptions": {
        "lib": ["DOM","ES5","ScriptHost", "esnext.promise"]
    }
}

test.ts:

// Polyfills es6-promise to the global object.
require('es6-promise').polyfill();

// Option 1: Don't import "es6-promise" as a module, because
//           it's available via the global Promise object anyway.

// Option 2: Import "es6-promise" as an untyped module, with
//           typings inferred from the global core libraries.
const Promise = require("es6-promise");
// (Comment out above line if you don't want to use option 2)

// Works, due to using typings from core library, including esnext.promise
Promise.race([]).finally();
// Works, due to using typings from core library, including esnext.promise
navigator.requestMediaKeySystemAccess().finally();

navigator.requestMediaKeySystemAccess()
.then(() => {
    // Works, due to using typings from core library, including esnext.promise
    Promise.resolve();
})
.catch((e: any) => {})
// Works, due to using typings from core library, including esnext.promise
.finally();
shirakaba commented 6 years ago

Have just contacted support, and found that WebStorm 2018.1 EAP (v181.4096.25 Early Access Program) supports esnext.promise, so there is no longer any need to add lib.esnext.promise.d.ts manually to one's own library when using WebStorm (it can be specified as a built-in lib in tsconfig instead). The problem of esnext.promise's typing for finally() not matching that of the core library's still stands, however.

lcoenen commented 6 years ago

Please see https://stackoverflow.com/questions/51174725/property-finally-is-missing-in-type-promise?noredirect=1#comment89332328_51174725

shirakaba commented 6 years ago

Please see my response: https://stackoverflow.com/a/51187968/5951226