When a native Promise implementation is available it should be left intact.

[fd]

I made a small demonstration of the problem. (test in Chrome)

http://jsbin.com/poxexupamo/1/edit?html,output

[briancavalier]

Hey @fd. Yes, it may be time to do this. It was a conscious decision to overwrite, for a few reasons. Early on, native promise implementations were slow, had bugs, and did not report unhandled rejections, so generally provided a very bad debugging experience. Thus, we felt it was more important to provide consistent speed, compliance, and unhandled rejection reporting across platforms even if native Promise was implemented.

We should reconsider, since native promises have come a long way. They're still slower, but no longer buggy, and some even report unhandled rejections upon garbage collection.

[briancavalier]

Does it seem kosher to do this in a 3.8.0 release? Or do you think it'd be considered a major breaking change, thus warranting a 4.0.0?

[briancavalier]

@fd and @guybedford Could you try the ES6 shim in the dont-overwrite-global-promise branch and let me know if it seems to work "correctly"? It should only overwrite global Promise if it doesn't exist. It should also still work as a requirable module (e.g. var WhenPromise = require('when/es6-shim/Promise')).

Thanks!

[unscriptable]

Does it seem kosher to do this in a 3.8.0 release? Or do you think it'd be considered a major breaking change, thus warranting a 4.0.0?

Good question. This is definitely going to break some apps.

[mikaelkaron]

I guess if you're truly semver and don't put any significant meaning to the version number (as in no "4.0.0 should be reserved for when we do major update" attitude) then this sounds like a 4.0.0 (as it could break existing apps on upgrade)

Just my 2c

[briancavalier]

@mikaelkaron Yeah, I think you're right. We should just go semver and not worry about any particular "big update" preconceptions wrt 4.0.0. I'll open an issue for collecting 4.0 related things, of which this will be one.

[guybedford]

@briancavalier just tested the non-global branch and it's working great for me.

[briancavalier]

Awesome, thanks for confirming, @guybedford

[AdamBrodzinski]

+1, overwriting breaks functionality for the Meteor pollyfill (uses fibers on the server) here's a use case

[briancavalier]

@AdamBrodzinski Hmmm, interesting. Does Meteor also overwrite global.Promise and so Meteor and when.js are colliding?

[AdamBrodzinski]

Does Meteor also overwrite global.Promise and so Meteor and when.js are colliding?

@briancavalier I believe so. Their use case for doing so is to enable fibers on the server so it's changing a lot under the hood. Then when is required, when.js overwrites, using it's own version instead of the promise.

The good part is the promise still works on the server but because it's not the Meteor version it lacks the await function which is used to return the the server RPC method (with that the client get's the RPC response when the promise resolves).

Eventually (from what I understand) they're going over to promise based APIs for async-await instead of fibers so that may change down the road. Perhaps @benjamn could comment on how the Meteor promise polyfill works?

[benjamn]

Meteor uses any existing globally defined Promise constructor on the server, and merely overwrites its .prototype.then method to ensure callbacks run in fibers. If the when library made a similar effort to reuse/patch the existing constructor, instead of replacing it, I believe everything would work from Meteor's perspective.

Code is here: https://github.com/meteor/promise/blob/master/promise_server.js

[briancavalier]

Thanks for the info @AdamBrodzinski and @benjamn. Do you think adding a simple noConflict() to when.js's shim, to restore the previous global, would be enough for a workaround in this case?

[AdamBrodzinski]

@briancavalier For this use case I was using the HelloSign SDK and they're using it in their lib. So from what I understand it would only be helpful if it was callable from outside the module after the require somehow.

[briancavalier]

@AdamBrodzinski Ah, that's a bummer. IMHO, polyfills are the domain of the application. Libraries really shouldn't inject language-level polyfills, since it makes combining libraries much more difficult.

The idea of noConflict is that it would be present on the when.js global Promise constructor that. IOW, someone can detect and call let WhenPromise = Promise.noConflict() and that will 1) restore the previous Promise global, and 2) return when's Promise constructor in case you want to hold onto it (the es6-shim/Promise module itself also returns that same constructor).

I fully admit it's not a great solution :) Just looking for alternatives.

The other option, and possibly the best option, is to release when.js 4.0, which would be identical to 3.7.3, but with a friendly polyfill.

[briancavalier]

@AdamBrodzinski I just took a quick look at the hellosign repo, thinking I'd open a discussion with them to see if they'd consider not loading the polyfill. I see them using when.js as a normal library, but not forcibly loading the polyfill. Are you using an older version which used the polyfill? Or maybe I'm missing it, and you can point me to the spot in their lib?

[AdamBrodzinski]

@briancavalier sounds good to me. I was using the latest. I can create a reproduction for you if it's helpful. I'm not sure exactly where it's being used.... I found the same spot you did. Here's the code I ran:

  var promise = hellosign.signatureRequest.createEmbedded(options)
    .then(function(response){
      var signatureId = response.signature_request.signatures[0].signature_id;
      return hellosign.embedded.getSignUrl(signatureId);
    })
    .then(function(response){
      return response.embedded.sign_url;
    });

  return promise.await();

and also if you don't call the await it won't throw (but doesn't return to the client RPC).

[briancavalier]

@AdamBrodzinski That example is very helpful, thanks!

It appears that there are actually 2 things going on wrt Meteor and when.js. A bit of background first. When.js's ES6 shim is a separate thing than when.js the package. The shim can and package can be loaded independently. The shim implements a closest-possible ES6-compliant Promise with no additional public methods. The package provides lots of functionality beyond the ES6 Promise spec.

1. Meteor and when.js's ES6 shim will collide. Yes, we should fix this :) However, that's not what appears to be happening here, based on the example code above, and based on scouring the Hellosign SDK to see if it's loading when.js's ES6-shim (which it doesn't appear to be).
2. Hellosign's SDK uses when.js the package, and returns when.js promises. In fact, they explicitly documents this. When.js promises have no method named .await() and have no knowledge of Meteor fibers. The example code is attempting to call .await() on a when.js promise, which appears to be why it fails.

It's only safe to call .await() on a promise that you know for sure is a Meteor-polyfilled promise.

A workaround for this case would seem to be to coerce the when.js promise (or any other non-Meteor promise, like one from Bluebird, RSVP, etc) into a Meteor promise using the global (and presumably Meteor-augmented) Promise.resolve. Again, given any promise that any library might return to you, that's the best way to know for sure that you have a Meteor promise in your hands.

 var promise = hellosign.signatureRequest.createEmbedded(options)
    .then(function(response){
      var signatureId = response.signature_request.signatures[0].signature_id;
      return hellosign.embedded.getSignUrl(signatureId);
    })
    .then(function(response){
      return response.embedded.sign_url;
    });

  // promise is from when.js
  // coerce it to a Meteor promise so we know for sure
  // it will have an await() method that does the right thing
  return Promise.resolve(promise).await();

Did all of that make sense? If you could give that a try and let us know what happens?

Thanks!

[benjamn]

That analysis makes sense to me! You can even avoid creating a new Promise object if you return Promise.await(promise), since the Meteor version defines the static Promise.await method to work for any kind of argument.

[briancavalier]

return Promise.await(promise)

Great tip, thanks @benjamn!

[AdamBrodzinski]

@benjamn @briancavalier Wow you guys are first class! Thanks for the explanation 🍻 I'll try this out Monday morning and will let you know either way!

I need to read up on the inner workings of promises too 😄

[sompylasar]

@briancavalier Any update on this please? It's been open for 1.5 years...