allow reverting to native Promise

[yoavniran]

I was quite surprised that on Chrome I saw that window.Promise was changed from the native one when I use this library as I intentionally didnt call the polyfill method. Then i saw the code:

Object.prototype.toString.call(P.resolve()) === '[object Promise]' && !P.cast)

and realized the issue as in Chrome this test fails (as reported multiple times #139 #136 #132 ...)

Normally I wouldnt care this much but Im working on a project in which my code is inserted into other websites and I dont wish to change anything on the global level and unfortunately using this library violates this rule.

Its my opinion that the explanation that the native imp may have issues/bugs is irrelevant for cjs/amd environments. The library shouldnt touch the global object unless explicitly requested., But if you still wish to leave it as it is, please give us the ability to revert back to the original/native Promise. a long the lines of noConflict.

Thanks!
Yoav

[stefanpenner]

Yoav the goal of this lib is a polyfil. In theory we can also ship a non polyfil version.

@jakearchibald you ok with that?

[yoavniran]

for amd/cjs in the browser I still think it should be more prudent not to force the polyfill on the global object.
However, I think the revert option is definitely a quick win as it should be easy to implement and doesnt change the current default behavior. I wouldnt mind preparing a PR if youd accept it(?).

thanks.

[stefanpenner]

I think we should just ship two builds. One that polyfils and one that doesn't. No?

[yoavniran]

sounds good to me. As long as the polyfill-less build also makes it into: https://github.com/components/es6-promise (as thats how I get this fine library) 😄

thanks!

[stefanpenner]

(as thats how I get this fine library)

ya thats plans

[yoavniran]

hey @stefanpenner any idea when you'll be able to release the non-polyfill version?

thanks.

[nenadvicentic]

@stefanpenner:

Can you, please, clarify your statement:

...goal of this lib is a polyfil. In theory we can also ship a non polyfil version.

Isn't the goal of polyfil to provide polyfilled API where native API is not yet implemented and fallback to native implementation where it already exists?

And more, if your goal is to override native implementation, why es6-promise behaves differently in Firefox (actually working properly...)?

Just to add some code context. Line of code:

Object.prototype.toString.call(Promise.resolve())

resolves in Google Chrome into:

"[object Object]"

while in Firefox it resolves into:

"[object Promise]"

in es6-promise source code there is a condition relying on "[object Promise]" string value:

  var P = local.Promise;

  if (P && Object.prototype.toString.call(P.resolve()) === '[object Promise]' && !P.cast) {
    return;
  }

  local.Promise = lib$es6$promise$promise$$default;

What's the purpose of this code, if not to fallback to native implementation (as it happens in Firefox)?

[skevy]

+1 on this issue - this also causes a problem on react-native.

[AlistairB]

A polyfill is supposed to be a fallback. If you have a working (obviously superior) native implementation the js polyfill shouldn't override it IMO.

[stefanpenner]

@AlistairB yes and in those situations the polyfil does not kick in. If the native implementation advertises itself in a non compliant manner there is no choice but to assume it is broken. And in fact.v8 promises do still have several issues.

[AlistairB]

Ah ok fair enough.

[stefanpenner]

I do plan to offer two builds. One as a lib and ones as a polyfil.

I have a patch for v8 as well. Maybe I'll work on submitting that this weekend.

[skevy]

I mean, Chrome certainly has a compatible promise implementation built in. There must be a way to detect it, right?

[skevy]

Having the polyfill function (as it is now) seems completely reasonable...but it just fails in places that actually do have compatible Promise implementations. In fact, on react-native (my use case), FB polyfills already with es6-promise (with one addon, which is a .done method)...but if you include a lib that ALSO polyfills with es6-promise, such as axios, it will overwrite it.

[eriktim]

The strange thing is the current check already assumes there is some kind of (non-native) Promise. If I define a dummy Promise object like window.Promise = {foo: 'bar'}; before running the polyfill it will crash.
I'd rather have a safe Promise check than having my native Chrome Promises be overwritten unnecessarily.

[stefanpenner]

I have decided to just fix v8:

first of several patches: https://codereview.chromium.org/1362773002

[mitranim]

In the user code, just check for the pre-existing Promise:

if (typeof window.Promise !== 'function') {
  require('es6-promise').polyfill();
}

[tilgovi]

Sure, I can check for it, but if I want to use this without polluting the global environment then I'd have to put that in every module I use Promise. Seems like it's better to export either the native or the polyfill, and have a separate submodule of this if you want it filled.

[yoavniran]

when I opened this issue I thought it was a simple one. I still do actually.

If I dont want the native implementation overridden I should be able to define that when using this lib.
Specifically when using in a all amd/cjs setup, I expect to have control over whether something leaks to the global object.

I still think a revert method or noConflict option is the best way to go. Or as was suggested, a version of the lib that doesnt touch the global object.

Honestly, I really dont understand why all this debate around the issue...

[tilgovi]

What about simply:

require('es6-promise') // returns platform Promise or polyfill without global pollution
require('es6-promise/polyfill') // return is irrelevant, window has Promise

[yoavniran]

@tilgovi thats exactly how I thought this lib worked until I found out that even requiring the promise module alone activated the polyfill... which started this thread... 😒

[Diokuz]

+1 here

I thought global variables in libraries is bad practice since the prototype js.

[Diokuz]

Also, I use this lib as subdeps of ssl-root-cas, and cannot change es6-promise behavior.

[tilgovi]

Same, @yoavniran

[remy]

Any progress on this issue? A polyfill, by definitely should not replace the native functionality if it's there.

So the following, by that definition, is a bug:

» node
> Promise
[Function: Promise]
> require('es6-promise')
{ Promise:
   { [Function: lib$es6$promise$promise$$Promise]
     all: [Function: lib$es6$promise$promise$all$$all],
     race: [Function: lib$es6$promise$promise$race$$race],
     resolve: [Function: lib$es6$promise$promise$resolve$$resolve],
     reject: [Function: lib$es6$promise$promise$reject$$reject],
     _setScheduler: [Function: lib$es6$promise$asap$$setScheduler],
     _setAsap: [Function: lib$es6$promise$asap$$setAsap],
     _asap: [Function: asap] },
  polyfill: [Function: lib$es6$promise$polyfill$$polyfill] }

Checked against 3.0.2.

[stefanpenner]

Any progress on this issue? A polyfill, by definitely should not replace the native functionality if it's there.

@remy it should replace, if the native functionality is broken.

[remy]

It's replacing node 4.2.1's native promises. Are you saying those are
broken?

On Wed, 25 Nov 2015 02:43 Stefan Penner notifications@github.com wrote:

Any progress on this issue? A polyfill, by definitely should not replace
the native functionality if it's there.

@remy https://github.com/remy it should replace, if the native
functionality is broken.

—
Reply to this email directly or view it on GitHub
#140 (comment)
.

[yoavniran]

@stefanpenner from everyone posting on this thread plus my own experience, a Polyfill shouldnt replace the native implementation as most dont (regardless of the actual native implementation detail).

As a consumer of this lib I should be able to decide for myself if I want the native Promise to be changed or not.

With the state of this code being widely available in the wild I still think and suggest there should be a way to at least bring back the native implementation as well as specify upfront that the native imp wont be replaced if that behavior isnt wanted.

From one of the posts here I think a different version/flavor of this lib without the aggressive override is less preferable as it means that if this library is a dependency of another library you're using, you wont be able to change the default behavior even if you wanted.

[fredefox]

Hello, I've tested this in Chrome Beta and the issue does not seem to be resolved by @stefanpenner's patch to V8.

From chrome://version:

Google Chrome: 48.0.2564.41 (Official Build) beta-m (64-bit)
JavaScript:    V8 4.8.271.8

I double-checked against V8 where git merge-base 4.8.271.8 dcbab gives me:

dcbab...

I conclude that I should have the patch. But Promise is still polyfilled.

If there is something I'm missing. Please advice.

[stefanpenner]

will be part of 4.0

[purtuga]

@stefanpenner
So its very clear from this thread that this library is NOT a polyfill. Your statement above:

@remy it should replace, if the native functionality is broken.

seem to imply that you are mostly interested in ensuring that any existing Promise in the executing environment conforms to the spec. That (IMO) is not the job of a polyfill... A polyfill should provide an implementation (which in itself could be the same or better than the native one) of the intended functionality if an implementation is not already provided - regardless if that implementation is native or not.

I applaud and thank you for your efforts in submitting code to v8 to fix their bugs (there should be more people like you), but that should not automatically change the meaning of a polyfill. Your implementation might be better than the native one, but the decision to use it, when one is already present in the environment, should not be automatically done by your library.

Re: Changes coming in v4.0
Even with your repeated statements that you will provide two versions with this library starting with v4.0, you still have not indicated whether your approach for determining "auto polyfill" will change. If the implementation remains the same, then again: this is not a polyfill.. you are providing a library that "possibly" overrides any existing implementation.

You condition for auto-polyfilll should be as simple as this:

  if (this.Promise) {
    return
  }

Where this points to the global object. (example from the .fetch() polyfill: https://github.com/github/fetch/blob/master/fetch.js#L4)

/Paul

[stefanpenner]

I believe you are incorrect. Polyfils typically also replace broken implementations. See core-js as an example

[purtuga]

I guess I could be - I'm just glad I read the issues before using the library as is. For my usuage, I have made local changes to ensure global.Promise is not overwritten if it already exists.

What will your approach/logic be with v4.0 as far as auto-polyfill goes? same as current ("deep checking")?

Thanks for the response and your work on this library.

I'll visit back when v4.0 is release.

/Paul

[stefanpenner]

What will your approach/logic be with v4.0 as far as auto-polyfill goes? same as current ("deep checking")?

Same or similar, check various things that are broken and patch if required. But autopolyfil itself wont be the default, so I suspect it wont be a pain in the ass as it is today

[purtuga]

👍

[nazrhyn]

As I just had a run-in with this module, I'd like to quote @yoavniran, from above (emphasis mine):

From one of the posts here I think a different version/flavor of this lib without the aggressive override is less preferable as it means that if this library is a dependency of another library you're using, you wont be able to change the default behavior even if you wanted.

I had already done...

global.Promise = require('bluebird');

...but, when I required the dropbox module, and it required es6-promise, bluebird was overwritten by the polyfill. Luckily, we don't actually intend to interact with Dropbox at this time; but, if we needed to in the future, how would we prevent this behavior? My only option would be to do something crazy like this:

let p = require('bluebird');
Object.defineProperty(global, 'Promise', {
    get: function () {
        return p;
    },
    set: function () {}
});

[dead-claudia]

I think it would make more sense to just make it a ponyfill, and offer a way to promote it to a polyfill. Magical things like these have a tendency of causing more problems than they solve in my experience, especially for library writers and users.

[dlong500]

It's been a while without any activity here. When is the 4.0 release that doesn't auto-polyfill supposed to come out?

[stefanpenner]

@dlong500 when its complete, and I have time to steward it through a release.

[stefanpenner]

build tooling for .auto vs lib variant for 4.0 PR #230

[yoavniran]

hi @stefanpenner is there any consideration made for depending on a library that uses the auto version when the auto behavior is not desired?

[stefanpenner]

@yoavniran no, because if a dependency truly depends on auto (which I would say is being a badly mannered dependency), then restoring the polyfil would break it.

I will make a note in the readme, that auto isn't intended for dependencies.

[yoavniran]

👍