Add .toPromise as analogy to .asCallback

[Ginden]

Very simple and useful addition:

Stream.prototype.toPromise = function() {
    return new Promise((resolve, reject) => {
       this.toCallback((err, res) => err ? reject(err) : resolve(res));
    });
};

Pros:

• Enables direct integration with APIs designed to work with promises
• Enables direct integration with language constructs like async/await

Cons:

• Won't work in Node 0.10 and Internet Explorer without Promise polyfill.

[vqvu]

You're not the first one to want this. See #289. I think the reasons articulated there for not having toPromise in the library still applies.

Note that it is fairly trivial to roll your own using through.

function toPromise(stream) {
  return new Promise((resolve, reject) => {
    stream.toCallback((err, res) => err ? reject(err) : resolve(res));
  });
}

const thisIsAPromise = stream.through(toPromise);

[Ginden]

I think the reasons articulated there for not having toPromise in the library still applies.

I'm sure there is no user who would want to use .toPromise method without depending on promises - especially when it's a part of language.

BTW compatibility with environments isn't stated in documentation (and Highland requires ES5 anyway).

[vqvu]

I'm sure there is no user who would want to use .toPromise method without depending on promises

It's about an having an implicit dependency within certain parts of the API on some global symbol that may or may not exist. It's part of the language, but (as you mention) not every relevant JS engine implements it.

Compatibility isn't explicitly stated, but we test against 0.10+ (so people can reasonably assume that we support it). And there is an effort made to support ES3 (though we don't test this, so I concede that this may not be the case). Regardless, ES5 doesn't define Promises, so I don't understand the point of your parenthetical.

That said, I would be fine with an implementation that does not have the implicit dependency. Essentially

Stream.prototype.toPromise = function(PromiseContructor) {
    return new PromiseContructor((resolve, reject) => {
       this.toCallback((err, res) => err ? reject(err) : resolve(res));
    });
};

PR welcome.

[angelf]

Why is not acceptable to sniff for global.Promise and if it's not there throw an exception?. Any user that calls .toPromise will be well aware that this requires a Promises implementation.

[vqvu]

I don't like adding an implicit dependency on something that is not always there, as that means correct usage of toPromise depends on the environment and not just on the local code. It's too easy for something to go wrong.

It would be different if Promises were core to the functionality of the library, since it would be clear that the library can't work at all without a Promise implementation. Requiring the user to pass in a Promise implementation all the time in such a library would significantly affect its usability. But what we have here is a fairly trivial, nice-to-have helper for interop with a Promises, so I am ok with taking a slight hit in usability in exchange for the removal of the implicit dependency.

[protometa]

It is odd for the library to readily consume promises without readily providing an optional way to transition back to them.

[vqvu]

I don't think it's so odd. Highland consumes a lot of things that it doesn't let you get back out (like arrays or Iterables). In fact, streams are strictly more powerful than promises (since they can handle multiple asynchronous values instead of just one), so I would expect it to be easier to convert from Promises to streams than the other way around.

Plus, I'm not saying we shouldn't have an easy way to convert back to a promise (toCallback was partly written to facilitate this). I'm just saying that since Promises aren't completely universal yet, users need to take the (imo minor) step of providing a Promise constructor when they convert.

[corbinu]

Just came across this project for an issue I was having, but I am very confused by this discussion @vqvu How are Promises not universal yet? There is no version of node that has been in security updates in over 6 months that does not support them.
https://github.com/nodejs/LTS

[svozza]

But there are tonnes of browsers that don't have them.

[corbinu]

@svozza Thats what https://github.com/stefanpenner/es6-promise is for. I have a whole bunch of webapps thats support all the way back to IE8 but have dependencies that depend on promises.
https://github.com/mzabriskie/axios is an example of such a dependency

[vqvu]

I understand that polyfills exist. They are not the problem. The problem isn't even an implicit dependency on a global Promise—not by itself anyway. As I've said before, there's nothing wrong with depending on Promise if it is core to the functionality of the library. Axios is a perfect example of such a library. Highland is not.

It comes down to a trade-off between developer user experience and implicit dependency on global state. For example, axios can choose to make all of its API require the user pass in a promise constructor, but that would be extremely annoying for their users. In their case, the improved user experience outweighs any issues that depending on Promise can cause.

For Highland, I think it's the opposite. toPromise is only useful when you need to interop with libraries or language features that require promises. It's use should be rare in comparison to other parts of the Highland API. The slight improvement in user experience from not requiring the promise constructor is not worth it. It's a value judgement. You may disagree.

Note that #628 was merged, so I'm not completely opposed to the idea of a toPromise method.

[corbinu]

That exactly my point though. You can easily make just toPromise be the only part of the library fail if promises are not available. So your hurting developer user experience for all Node developers and for a very large segment of browser developers who either A) don't target old IE (4% of the usage) or B) Already pollyfill for some other dependency.

Adding the feature does not affect the experience developers who don't use promises in the slightest. Not adding it directly affects developers who do use Promises/Async/Await. You just add a note that says "Hey don't call the .toPromise method if your not sure Promises exist your app will crash"

[vqvu]

I don't know what else I can say except, I disagree. I just don't think it hurts usability all that much. We're talking about

stream.toPromise(Promise)

instead of

stream.toPromise()

here. And having an API that works the same everywhere is not worth nothing. There is some line beyond which the latter trumps the former, and I think that's where we are.

[corbinu]

Ok no worries ... that's the good news about software is we can always disagree. Like I said in my post. I was simply confused on why this was an issue (That mostly revolved around the issue of why Node 0.10 and IE8 support without a pollyfill should matter for anything not promise specific).

Honestly at this point I just took a couple days and implemented what I needed myself. Best of luck.

[darky]

#630 Here ability to use .toPromise() without passing Promise (it default)

[vqvu]

Released as 2.11.0/3.0.0-beta.5

[darky]

stream.toPromise(Promise) is API design, that smell bad. Library should by default use Promise constructor and don't require to pass Promise noise in 90% cases. For "old fags", who don't have native Promise support need to add ability to setup own Promise constructor. It can be passed directly in .toPromise as now or/and global setup like https://marionettejs.com/docs/v2.4.4/marionette.configuration.html#overriding-marionettedeferred

[vqvu]

I am not particularly interested in continuing to beat a dead horse. I've already explained my rationale behind the toPromise(Promise) signature. If you disagree, you are free to implement your own transform via one of the following methods

1. As a through-transform
2. By directly patching the Highland prototype (not recommended, and we won't support any issues that arise).
3. With the new use feature in the 3.0.0 beta (npm install highland@next).

   const Highland = require('highland');
   const _ = Highland.use({
     toPromise() {
       return Highland.toPromise(Promise, this);
     }
   });
   
   _([1]).toPromise()
     .then(x => console.log(x));  // => 1

   Sorry that documentation for it doesn't exist yet.

Edit Breaking changes in 3.0.0 are documented in CHANGELOG.md.

[rightaway]

Could anyone provide a practical example on when toPromise could be used? I'm using highland and am curious if I could simplify my existing code by using this.

[vqvu]

I would say any of the uses of promises that you bring up in #593 could be improved slightly with toPromise.

Usages of

new Promise((resolve, reject) => {
  stream.someTransform(...)
      ...
      .stopOnError(reject)
      .done(resolve);

could be replaced with

stream.someTransform(...)
    ...
    .stopOnError((e, push) => push(e))
    .filter(x => false)
    .toPromise(Promise);

or, if you convert to promises often, with

// Defined once.
function toCompletedPromise(stream) {
  return stream.stopOnError((e, push) => push(e))
      .filter(x => false)
      .toPromise(Promise);
}

stream.someTransform(...)
    ...
    .transform(toCompletedPromise);