Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Reassign global Promise to es6-promise if Promise.prototype.finally() is undefined. #331

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shirakaba
Copy link

Fixes #330 .

@shirakaba
Copy link
Author

shirakaba commented Mar 13, 2018

If anyone needs this patch before it is reviewed (or wants to review it without a git checkout), they can add a function called patchedPolyfill() to the es6-promise module and invoke that instead:

const ep = require('es6-promise');

ep.patchedPolyfill = function(){
    let local;

    if (typeof global !== 'undefined') {
        local = global;
    } else if (typeof self !== 'undefined') {
        local = self;
    } else {
        try {
            local = Function('return this')();
        } catch (e) {
            throw new Error('polyfill failed because global object is unavailable in this environment');
        }
    }

    let P = local.Promise;

    if (P) {
        var promiseToString = null;
        try {
            promiseToString = Object.prototype.toString.call(P.resolve());
        } catch(e) {
            // silently ignored
        }

        if (promiseToString === '[object Promise]' && !P.cast){
            if (typeof P.prototype.finally !== "undefined"){
                return;
            }
        }
    }

    local.Promise = ep;
};

// returns 'undefined', in desktop Safari (but likely not all browsers)
console.log(`typeof Promise.prototype.finally, before polyfill:`, typeof Promise.prototype.finally);
ep.patchedPolyfill();
// returns 'function'.
console.log(`typeof Promise.prototype.finally, after polyfill:`, typeof Promise.prototype.finally);

@stefanpenner
Copy link
Owner

@jakearchibald would love you input on this.

@sherdeadlock
Copy link

It can polyfill only finally instead of Promise object.

@shirakaba
Copy link
Author

shirakaba commented Apr 18, 2018

I suppose what would be ideal would be to add some configuration to the polyfill function. Here are two options (presented using TypeScript-style interfaces) for comment:

// Option #1: Simply opt-in to ALL features beyond ES6/ES2015
interface Polyfill {
    // DEFAULT: polyfill(true, true);
    polyfill(beyondES2015?: boolean, asAugmentation?: boolean): void;
}

// Option #2: Opt-in to a specific ECMAScript version.
interface Polyfill {
    // DEFAULT: polyfill("ES2018", true);
    polyfill(ESVersion?: "ES2015"|"ES2018"|"next", asAugmentation?: boolean): void;
}

Options

1: Simply opt-in to ALL features beyond ES6/ES2015

Nice because it's simple.

2: Opt-in to a specific ECMAScript version.

Helpful because it allows users to polyfill only an ECMAScript version of this library that would be in line with the rest of their libraries.

Issues with both

Any beyond-ES6/ES2015 feature will have to be written as optional into our distributed TypeScript typings, and all TypeScript users will have to mark usages of finally as explicitly non-optional (unless, like me, they ditch the typings altogether and just set up their tsconfig.json to include the appropriate lib option, e.g. "ES2015.Promise" or "ES2018.Promise").

asAugmentation flag

This is the relevant part to this issue filing. If a Promise implementation is already present, we need to decide whether to replace it altogether or just augment it with this library's extra features like Promise.prototype.finally.

@sherdeadlock is suggesting to just augment it. If we're confident that inter-op with native implementations of Promise can be assured, then this flag should default to true.

If we're not so confident, it should default to false, meaning that the native Promise implementation should be altogether replaced with that of this library.

Side-note

As @stefanpenner says, this module really shouldn't be called es6-promise!


I can implement whatever is desired, but I shall first require a conclusion on which solution is preferred.

@shirakaba
Copy link
Author

Alternatively, instead of adding parameters to polyfill(): we rename this repo to es-promise and just force polyfilling of all the latest features like Promise.prototype.finally.

Also: I just realised that if we parameterised polyfill(), then the var Promise = require('es6-promise').Promise; API would become confusing (as it implicitly provides all the latest Promise features. require('es6-promise/auto') would also need to be supplemented by a require('es6-promise/justES6'). Looking a bit messy...

@jpike88
Copy link

jpike88 commented May 18, 2018

its been two months.. any ideas here?

@YurySolovyov
Copy link

@stefanpenner is there anything we can do without @jakearchibald's input in the mean time?

@guilipan
Copy link

this feature is currently in TC39 stage4,why this request hasn't been merged yet?

@pbastowski
Copy link

Hi Guys
Any chance this will get merged? What's preventing it from being merged?

@stefanpenner
Copy link
Owner

stefanpenner commented Jul 19, 2018

we rename this repo to es-promise

Although this is semantically accurate, Im not sure its super valuable. I suspect if we nail the details here, we can simply have es6-promise's uses benefit... Kinda sucks if you gotta choose different modules entirely to get what you want.

Also: I just realised that if we parameterised polyfill()

This may be a path forward, some random thoughts:

require('es6-promise/es-2018'); // forces finally
require('es6-promise/ensure-finally'); // forces finally

@shirakaba
Copy link
Author

@stefanpenner The conundrum I was wondering about mainly was: If the browser doesn't have Promise.prototype.finally already, should we:

  1. Augment it just with Promise.prototype.finally (using the – presumably – highly-optimised native version of Promise, but having the unforeseen problems of running a chimaeric library); or

  2. Replace the browser's whole Promise implementation with that of es6-promise (which ensures consistency of the whole implementation)?

Currently my PR does 2). Do users have any preference towards 1)?

@YurySolovyov
Copy link

What are chances of getting a browser with native Promises, but without finally?

@pbastowski
Copy link

pbastowski commented Jul 19, 2018

Chances are good. Chrome v66 and a recent version of FireFox didn’t have it. We had to poly fill with Bluebird, because es6-promise didn’t work.
Chrome 67 does support finally.

@YurySolovyov
Copy link

I'd go with granular approach then (see what i did? :D)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants