Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

include proposed Promise.prototype.finally #232

Closed
stefanpenner opened this issue Sep 12, 2016 · 13 comments
Closed

include proposed Promise.prototype.finally #232

stefanpenner opened this issue Sep 12, 2016 · 13 comments

Comments

@stefanpenner
Copy link
Owner

stefanpenner commented Sep 12, 2016

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:

  • should 4.0.0's es6-promise/auto polyfill the whole promise, or just finally if finally is not present?
  • should we wait a-bit longer or is the risk sufficiently low to just :shipit: cc @domenic
@domenic
Copy link
Contributor

domenic commented Sep 12, 2016

/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 tc39/proposal-promise-finally#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
Copy link
Owner Author

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

@ljharb
Copy link
Contributor

ljharb commented Sep 12, 2016

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

@stefanpenner
Copy link
Owner Author

stefanpenner commented Sep 12, 2016

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

@elodszopos
Copy link

Looking forward to this one boys!

@shirakaba
Copy link

shirakaba commented Mar 12, 2018

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
Copy link
Contributor

ljharb commented Mar 12, 2018

@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
Copy link

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
Copy link

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
Copy link

shirakaba commented Mar 13, 2018

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
Copy link

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.

@shirakaba
Copy link

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

6 participants