nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
104.97k stars 28.42k forks source link

Feature Request: Every async function returns Promise #11

Closed rdner closed 9 years ago

rdner commented 9 years ago

Now in node we have callback or EventEmitter model to deal with async calls by default. But in my opinion it is better if every async function returns a native Promise (from new version of V8). It does not break backward compatibility and supports optional callback if needed.

If so we just do not need to install additional package for promises like q or bluebird except additional functionality is needed.


_EDIT 2014-12-11 by @rvagg_

Comment lifted from here so it's easier to see for newcomers to this conversation.

This was discussed at the TC meeting yesterday, see #144, the aim was to be able to provide at least some kind of statement as feedback in this issue. I don't think the issue needs to be closed and can continue to collect discussion from those who feel strongly about this topic.

The feedback from the TC about incorporating a Promises-based API in core goes something like this:

A Promises API doesn’t make sense for core right now because it's too early in the evolution of V8-based promises and their relationship to other ES* features. There is very little interest within the TC in exploring this in core in the short-term.

However, the TC is open to change as the feature specifications and implementations in ES6 and ES7 are worked out. The TC is open to experimentation and providing the most optimal API for users, which may potentially include a Promises-based API, particularly if newer features of JavaScript work most optimally in conjunction with Promises. The speed of the language specification process and V8 implementation will mostly dictate the timeline.

It should be noted that a callback API is unlikely to ever go away.


drewhamlett commented 9 years ago

@rvagg Thanks for your write up!

arcanis commented 9 years ago

Dang, see you later after ES6 release for the next round :)

benjamingr commented 9 years ago

@rvagg sounds like the reasonable and responsible decision to me - thanks for the update!

tracker1 commented 9 years ago

I don't think it really needs to modify the core... require('fs') vs. require('promise-fs) should be enough... the core codebase can still be callback based, it's easy enough to wrap...

cscott commented 9 years ago

@tracker1 https://www.npmjs.com/package/pn lets you do require('pn/fs') instead of require(fs). I'm interested in hearing feedback from its use -- especially the decisions made about wrapping the "difficult" node API methods (where there is both a callback and a return value, or the crypto functions). I would hope that experience using packages like pn would help inform the Promise API used by core node, if/when that happens in the future.

alessioalex commented 9 years ago

-1 Promises don't belong in core (just yet).

jonathanong commented 9 years ago

i currently have https://github.com/normalize/mz, which is like @cscott's package.

an idea is to have two versions of every core model, a regular version and a "next" version. the "next" version could include new, crazy, experimental features and would be completely opt-in. it could also be either part of node (i'd prefer it) or a separate module like mz, but either way ideally blessed by node and added to the io/ organization.

another example is https://github.com/timoxley/npm-next

greim commented 9 years ago

What about returning a thunk, but only if the callback is omitted? Thus these would be identical:

fs.readFile('foo.txt', function(err, data){ ... });
fs.readFile('foo.txt')(function(err, data){ ... });

But a thunk could be trivially converted to a promise at creation time.

jmar777 commented 9 years ago

@greim Thunks make for a nice, simple paradigm, but they inhabit an awkward place in node-land. Basically they were only popularized by co, and saw very little adoption prior to that (although other libraries previously called them "continuations" on occasion).

I think there are two main issues with thunks in core, though:

  1. All the arguments about "opinionatedness" that have previously been applied to Promises in core, still apply to thunks. *
  2. While thunks are friendly for generator-function-as-co-routine runners (a la co and suspend), they won't be future-compatible with ES7 async / await syntax, which is what I at least hope everyone agrees is a more desirable eventual target for core APIs.

* The only reason I think we can lift the "opinionatedness" argument against Promises at this point is because they're an actual language construct now, with future language-level syntax sugar built around them. In fact, at this point callbacks can probably be considered more opinionated than Promises, simply because they're framework/library-level constructs, not language-level.

jonathanong commented 9 years ago

don't include co in the conversation with thunks. thunks only remain in co for backwards compatibility :)

jmar777 commented 9 years ago

@jonathanong Good point. Next version of suspend is yanking support for them completely as well.

rlidwka commented 9 years ago

Callbacks are simple. You supply a function, library calls it later. That's all.

Only disadvantage is that "callbacks-are-the-last-argument" convention is too complex, and generic libraries that do not know how many arguments function have can't work with it.

But there is where chunks come out. They are always one-argument functions, which call a function supplied to them. Simple.

Promises with that verbose .then() and .catch() syntax and a complex prototype sound way too heavy. And only thing they solve is error handling, but current es6 promises don't even provide that.

Sorry, but I'd rather see chunks everywhere.

drewhamlett commented 9 years ago

@rlidwka I don't think your quite grasping promises. It's not just about error handling. It's about chaining.

redis.getAsync('users').then(JSON.parse).then(fetchPosts).then(renderResponse).catch(handleError);
rlidwka commented 9 years ago

@drewhamlett ,

  1. I saw people argue that nested callbacks are hard to maintain, so promises are better. So now we have promises in ES6.
  2. I saw people argue that promises are hard to maintain, and synchronous code is better. So now we have async/await in ES7.

But... if we will have async/await anyway, what are promises good for?

We don't need chaining. It is covered by generators in co, and it will be covered by await construct later. As for now, async library does the job just fine.

greim commented 9 years ago

@rlidwka - I agree. In a hypothetical world where async/await (or even just yield) existed from the start, it doesn't seem like anyone would have thought "hey, we need promises now." I could be wrong, but promises feel like an interim solution whose momentum is mainly due to the suckiness of ES5.

drewhamlett commented 9 years ago

@rlidwka Promises are still good to me. I have async / await in my project right now using 6to5 and I still use Promises all over the place.

There are people that actually prefer error handling with Promsies and even regular node callbacks with the error param over try catch mess. Just use Java and you'll see. I'd take regular node callbacks for error handling any day over Java with try catch all over the place, and I hate callbacks.

Also async and await expect promises, so don't think they'll disappear anytime soon.

Generators are a hack and so is all this co and thunk crap. While they may be an option in the short term, async / await with promises is the way the language will go.

Edit: I actually agree with @rlidwka and @greim . If the language had async await in 1995, I'm not sure anyone would have used Promises.

reqshark commented 9 years ago

@rlidwka +1

@drewhamlett -1

Edit: @drewhamlett -0

cscott commented 9 years ago

I think you're missing the point that await/async are built on Promises. So you can only use await/async after you have some mechanism to return promises from the node code API.

rlidwka commented 9 years ago

I think you're missing the point that await/async are built on Promises.

There is no reason why await should depend on Promises. So, I expect more discussion about this in es-discuss later on.

And while "await/async are built on Promises" is a valid argument for now, I surely hope it isn't the only argument to use them.

cscott commented 9 years ago

@rlidwka My point is that you're begging the question: if we adopt await/async, what does the node core API look like? I am reporting from the current ES6/ES7 process and telling you that it looks like the node core API would be returning promises, hence issue #11.

If you say, no -- let's use something else for the node core API, but also hook it up to await/async -- well then you need to tell us what that something else is. It's not callbacks.

The pn/mz/npm-next approaches have the beautiful property that a single API can support both callback-style programming and promise/await-async. If you propose to build await/async on some other primitive, then (a) I eagerly await your proposal to es6-discuss for details, and (b) you still need to explain how you are going to extend the node API to support your primitive while still providing backward compatibility for users of callbacks.

jmar777 commented 9 years ago

re: @rlidwka

There is no reason why await should depend on Promises. So, I expect more discussion about this in es-discuss later on.

I suppose specs aren't final until they're final, but this really hasn't been open for discussion at all since very early on in the async/await specification process. In fact one of the reasons it was punted to ES7 was so that the Promise API could stabilize during ES6. Based on virtually every es-discuss thread or TC39 meeting notes available, it's pretty safe to say that Promises will definitely be the primitive upon which async/await builds on top of.

And while "await/async are built on Promises" is a valid argument for now, I surely hope it isn't the only argument to use them.

The other arguments for Promises over callbacks or thunks are usually more subjective or preferential (which isn't to say invalid), but the fact that ES6 has an actual language construct for representing eventual completion, and ES7 is slated to have syntax niceties around that construct (combined with the high likelihood of additional love from engines and developer tools), the only thing that callbacks have going for them at this point is the fact that they're already there. It doesn't matter that they're simple or actually a lot easier to use than the horror-story blog posts make them out to be. Even the most casual observation of the language's evolution indicates that callbacks will eventually just be something that node did before the language had Promises.

greim commented 9 years ago

the fact that ES6 has an actual language construct for representing eventual completion, and ES7 is slated to have syntax niceties around that construct

This is compelling reasoning. I still worry that promises will be nothing more than GC fodder and useless overhead at first since existing code uses callbacks. What about only returning a promise if the callback is excluded?

jmar777 commented 9 years ago

What about only returning a promise if the callback is excluded?

That's definitely been discussed. The API challenge with that is a few variadic functions (in the crypto module, for example) that actually switch to synchronous execution when the callback is omitted.

spion commented 9 years ago

@greim closure functions have a huge cost anyway. Well optimized promises don't add too much to it. But if node added support for context passing to callback-based functions, promise libraries + generators could utilize that really well (the stuff with a -ctx suffix)

petkaantonov commented 9 years ago

And it's not a totally strange idea, all the ES5 array methods support context passing (except reduce for some reason)

jmar777 commented 9 years ago

Not the ultimate of concerns, but it's worth at least noting that context passing is hostile to arrow functions.

tracker1 commented 9 years ago

@jmar777, not a big fan of using context/this, tend to prefer binding with initial context argument(s)... More a matter of taste though.

spion commented 9 years ago

This shouldn't matter at all as normally end users will never pass context objects. They would only be used to implement abstractions such as promises/generators/etc efficiently on top of them (or for other performance-sensitive code)

bjouhier commented 9 years ago

@rlidwka To be comfortable, you need two mechanisms:

I have been using this model for almost 4 years now (with futures and streamline syntax) and it really works very well. If you only have await, you'll be lacking a clean way to parallelize.

 // sync-style: wait on promise immediately
var foo = await bar();

// sync-style + parallelization:
var fooP = bar(); 
doSomeStuffWhileBarIsRunning();
doMoreStuffWhileBarIsRunning();
// ...
var foo = await fooP;

So you need some form of promise to hold a computation in progress but what you don't really need is the promise API. So I agree with you that then does not bring much once you have async/await:

// chaining with then
result = await f1().then(x => x.f2()).then(x => x.f3());
// chaining with await
result = await (await (await f1()).f2()).f3();

I find the second form easier to understand but it is unfortunate that await was designed as a prefix operator because it forces you to have clumsy parenthesizing. Compare with the following:

// chaining with streamline
result = f1(_).f2(_).f3(_);
// chaining with hypothetical ! await operator
result = f1!().f2!().f3!();

I know that this is only sugar. But sometimes sugar matters.

drewhamlett commented 9 years ago

You guys feeling jelly?

import Promise from 'bluebird';
const fs = Promise.promisifyAll(require('fs'));

const current = async function (req, res) {

  var file = await fs.readFileAsync('package.json');
  var user = await knex('users').first();

  if (!user) {
    return res.json(404);
  }

  res.json({
    user: user,
    file: file.toString()
  });
};

or better yet

var data = await Promise.props({
  file: fs.readFileAsync('package.json'),
  user: knex('users').first()
});

https://github.com/6to5/6to5

benjamingr commented 9 years ago

@drewhamlett so you're telling us this problem is already solved very well in user level code? That's pretty cool :)

Also, you can do on top:

var {all} = Promise 

And then do:

  var [file, user] = await all(fs.readFileAsync('package.json'), knex('users').first());

As a more elegant alternative to your .props call

qfox commented 9 years ago

I don't know why you want to break anything just because ES7 have some async/await operators. But if community will go to ES7 — there is no reason to stay away and live with node-callbacks as the last param.

Also I'm surprised that nobody suggest a separated method like somethingSync but with promised result:

fs.readPromised('/dev/random').then(...)

But anyway, I believe that we shouldn't broke anything before it will be a standard and native promises will show us the same performance as usual callbacks.

Edit: Forgot to say: also we can rename current core methods to somethingClassic and put promised versions to their places.

ijroth commented 9 years ago

Referencing Support promises in the 'fs' module and its discussion

tracker1 commented 9 years ago

@zxqfox Personally, given that a function can have properties, it bugs me to see things like fs.readSync over vs.read.sync or fs.read.promise as alternatives... simply chaining the default method when you want a different interaction/call.

gabrielmancini commented 9 years ago

I like callbacks i feel free to build my own flow... 2 cents

mikeal commented 9 years ago

Any feature request like this is likely to get derailed by partisanship and concerns over performance.

The most productive approach to this would probably be to explore ways of simultaneously supporting callbacks and promises without a performance impact, perhaps with the help of new v8 optimizations. If someone were to discover a way to do this it would be hard to justify not supporting both APIs. If it's not possible, and we have some proof of that via experimentation, then we can move on to a conversation about what the performance impact is and if and when it is justifiable to eat it.

@domenic any thoughts on where to start?

piscisaureus commented 9 years ago

The most productive approach to this would probably be to explore ways of simultaneously supporting callbacks and promises without a performance impact

@sam-github and @bajtos have been measuring different strategies:

sam-github commented 9 years ago

sam-github/node#1 is closed, and noisy, I PRed to iojs, for discussion and record: https://github.com/iojs/io.js/pull/688

bjouhier commented 9 years ago

@mikeal The proposal is to have the functions return a promise when the callback parameter is omitted, and to keep the current behavior when the callback parameter is present. This can be implemented with a single line change in the async functions:

if (!cb) return promisified(currentFunction, this, arguments);

The overhead is a single test when the function is invoked with a callback. The performance argument is moot. It's just an ideological scarecrow.

FWIW, this proposal has been on the table for almost 4 years (with futures aka thunks instead of promises)

zowers commented 9 years ago

is it a good idea to change those varadic crypto functions to accept Promise constructor instead of callback to denote that promise must be returned?

benjamingr commented 9 years ago

@zowers why would anything except a promise constructor?

zowers commented 9 years ago

@benjamingr not sure I understood your question I suggested to teach crypto.randomBytes() to return a Promise if called with Promise as second arg:

crypto.randomBytes(4, Promise)
.then(console.log)

that would be an answer to the question how to choose between sync and promise-returning versions of functions which have optional callback api

phpnode commented 9 years ago

If you need to make a sync function return a promise you can just use Promise.cast(), passing the promise constructor isn't idiomatic

benjamingr commented 9 years ago

@phpnode Promise.cast doesn't actually (officially) exist anywhere :D You mean .resolve?

phpnode commented 9 years ago

@benjamingr whoops, you're right of course, that's a bluebirdism, I meant .resolve()

benjamingr commented 9 years ago

@phpnode .cast has been deprecated in bluebird since 1.0 IIRC, over a year ago :P

phpnode commented 9 years ago

@benjamingr damn, serves me right for not reading CHANGELOGs

impcyber commented 9 years ago

2015-02-03 17:18 GMT+03:00 Benjamin Gruenbaum notifications@github.com:

@phpnode https://github.com/phpnode .cast has been deprecated in bluebird since 1.0 IIRC, over a year ago :P

— Reply to this email directly or view it on GitHub https://github.com/iojs/io.js/issues/11#issuecomment-72657202.

bjouhier commented 9 years ago

@benjamingr @phpnode How do you distinguish a promise constructor from a callback? They are both functions.

zowers commented 9 years ago

@bjouhier it has Promise.resolve() and Promise.reject()