Adding Core support for Promises #5020

Closed
wants to merge 33 commits into
from

Conversation

Projects
None yet
@chrisdickinson
Contributor

chrisdickinson commented Feb 1, 2016

Update 02/02/16 15:07:00 PST: Current state of the PR.
Update 02/01/16 14:10:00 PST: Current state of the PR.

Newcomers: As a heads up, GitHub has started clipping the thread down. I'm going to start cataloguing common questions tonight, and I'll include them under this notice so that you can quickly look to see if there's an answer to your issue without having to search through the entire thread. Thanks for taking a look & weighing in! Whether you're for this change or against it, your feedback is valuable and I'll respond to it as soon as I'm able.

FAQ / TL;DR for newcomers

What is the current proposal?

If landed, this will land behind a flag, moving out from behind the flag in the next major version as an unsupported API, finally becoming fully supported one major version after that. It does not replace or supersede the callback API, which will continue to be the canonical Node interface.

Promise-returning variants will be exposed for all "single-operation" Node methods — that is to say, Streams and EventEmitter-based APIs are not included in this discussion.

To use promises:

const fs = require('fs')

fs.readFile.promise(__filename).then(data => {
})

Using promises with destructuring assignment (where available):

const {readFile} = require('fs').promise

readFile(__filename).then(data => {
})

As a prerequisite to this PR landing, domains must work as expected with native Promises (they currently do not.) AsyncWrap is not blocked on this PR, neither is this PR blocked on AsyncWrap. Both PRs are blocked on getting adequate instrumentation of the microtask queue from V8 (we need a callback for "microtask enqueued" events so we can track their creation, and ideally a way to step the microtask queue forward ourselves, running code between invocations.)


Why use the promisify approach?

Response summed up here.


Why not fs.readFileAsync?

@phpnode notes that it could break existing users of bluebird.promisifyAll. Plus, the naming is contentious.


Why not return promises when callbacks are not given?

  • @jasnell expresses deep discomfort with switching return types.
    • We already have to provide alternate APIs in some places due to

Should programmer errors throw, or reject?

Summed up here.


What is the value in adding them to core?

@rvagg has been massively helpful in driving this conversation forward:


How do you address the need to keep core small while expanding the exposed API?

Where do we draw the line in supporting language-level constructs?

@yoshuawuyts, @rvagg, and @DonutEspresso have largely driven this conversation:


Why are modules (like require('fs/promise')) not in scope for this discussion?

@phpnode, @RReverser, @mikeal, @ktrott, (and others, I'm sure!) have brought this up, and I've been pushing to have this conversation in a different, later issue. The conversation has mainly revolved around consensus, and the difficulty in attaining it when adding modules to the mix.


This is the original text of the PR. It does not reflect the current state of the conversation. See above for common themes!
This PR introduces Core support for Promises. Where possible, callbacks have
been made optional; when omitted, a Promise is returned. For some APIs (like
http.get and crypto.randomBytes) this approach is not feasible. In those
cases promise-returning *Async versions have been added
(crypto.randomBytesAsync). The naming there isn't set in stone, but it's
based on how bluebird's promisifyAll
method generates names.

This PR allows users to swap the implementation of promises used by core.
This helps in a few dimensions:

  • Folks that prefer a particular flavor of Promises for sugar methods can set
    the promise implementation at app-level and forget about it.
  • This should alleviate concerns about stifling ecosystem creativity — putting
    promises in Core doesn't mean Core needs to pick a winner.
  • Folks who prefer a faster implementation of promises can swap in their
    preferred library; folks who want to use whatever native debugging is added
    at the V8 level for native promises can avoid swapping in promises.
  • A world of benchmarks opens up for Promise library authors — any code in the
    ecosystem that uses promises generated by core and has a benchmarking suite
    is a potential benchmark for promise libraries.

The API for swapping implementations is process.setPromiseImplementation().
It may be used many times, however after the first time deprecation warnings
are logged with the origin of the first call. This way if a package misbehaves
and sets the implementation for an application, it's easy to track down.

Example:

process.setPromiseImplementation(require('bluebird'));

const fs = require('fs');

fs.readFile('/usr/share/dict/words', 'utf8')
  .then(words => console.log(`there are ${words.split('\n').length} words`))
  .catch(err => console.error(`there are no words`));

Streams are notably missing from this PR – promisifying streams is more
involved since the ecosystem already relies on a common (and more importantly,
often pinned) definition of how that type works. Changing streams will take
effort and attention from the @nodejs/streams WG. All other callback-exposing
modules and methods have been promisified (or an alternative made available),
though.

Three new internal modules have been added:

  • lib/internal/promises — tracks the original builtin Promise object, as
    well as the currently selected Promise implementation. All promises
    created by Node are initially native, and then passed to the selected
    implementation's .resolve function to cast them.
  • lib/internal/promisify — turn a callback-accepting function into one that
    accepts callbacks or generates promises. Errors thrown on the same tick
    are still thrown, not returned as rejected promises — in other words,
    programmer errors, such as invalid encodings, are still eagerly thrown.
  • lib/internal/callbackify — turn a synchronous or promise-returning function
    into a callback-accepting function. This is only used in lib/readline for
    the completer functionality. This could probably fall off this PR, but it
    would be useful for subsequent changes to streams.

Breaking Changes

  • In general: any API that would have thrown with no callback most likely does
    not throw now.
  • fs APIs called without a callback will no longer crash the process with an
    exception.
  • ChildProcess#send with no callback no longer returns Boolean, instead
    returns Promise that resolves when .send has completed.
  • Wrapped APIs:
    • child_process.ChildProcess#send
    • cluster.disconnect
    • dgram.Socket#bind
    • dgram.Socket#close
    • dgram.Socket#send
    • dgram.Socket#sendto
    • dns.lookupService
    • dns.lookup
    • dns.resolve
    • fs.access
    • fs.appendFile
    • fs.chmod
    • fs.chown
    • fs.close
    • fs.exists
    • fs.fchmod
    • fs.fchown
    • fs.fdatasync
    • fs.fstat
    • fs.fsync
    • fs.ftruncate
    • fs.futimes
    • fs.lchmod
    • fs.lchown
    • fs.link
    • fs.lstat
    • fs.mkdir
    • fs.open
    • fs.readFile
    • fs.readdir
    • fs.readlink
    • fs.realpath
    • fs.rename
    • fs.rmdir
    • fs.stat
    • fs.symlink
    • fs.unlink
    • fs.utimes
    • fs.writeFile
    • net.Socket#setTimeout
    • readline.Interface#question
    • repl.REPLServer#complete
    • zlib.deflateRaw
    • zlib.deflate
    • zlib.gunzip
    • zlib.gzip
    • zlib.inflateRaw
    • zlib.inflate
    • zlib.unzip

New APIs

  • process.setPromiseImplementation
  • net.connectAsync
  • tls.connectAsync
  • crypto.randomBytesAsync
  • crypto.pbkdf2Async
  • crypto.pseudoRandomBytesAsync
  • crypto.rngAsync
  • crypto.prngAsync
  • http.getAsync
  • https.getAsync
  • http.requestAsync
  • https.requestAsync
  • child_process.execAsync
  • child_process.execFileAsync

Next steps

I haven't finished this PR yet: the primary missing piece is tests for the
promise-based APIs, to ensure that they resolve to the correct values. Docs
also need updated. I'll be working on this in my free time over the next week.

Here are the bits that I'd like folks reading this to keep in mind:

  • The code changes here are fairly cheap, time-wise — one evening's worth of work.
    • Technical possibility has never been the primary blocker.
  • We know async/await is coming down the pipe, and many devs are interested in
    it.
    • With ChakraCore potentially coming in, this may happen sooner than anyone
      previously imagined.
    • Even sans the ChakraCore PR, it's my understanding that V8 will be supporting
      async/await by EOY (Correct me if I'm wrong, here!)
  • Promises may not be your cup of tea. This is okay. This is not an attempt to
    replace callbacks or streams with promises. They can co-exist. While Promises
    are complicated, much of that complication falls out of the problem space that
    both callbacks and promises abstract over. Give this a fair shake.
lib/internal/promisify.js
+ case 1: return resolve();
+ case 2: return resolve(arguments[1]);
+ }
+ return resolve([...arguments].slice(1));

This comment has been minimized.

@chrisdickinson

chrisdickinson Feb 1, 2016

Contributor

eslint chokes on ... — it may need upgraded?

@chrisdickinson

chrisdickinson Feb 1, 2016

Contributor

eslint chokes on ... — it may need upgraded?

This comment has been minimized.

@targos

targos Feb 1, 2016

Member

try to add spread: true to ecmaFeatures in our eslint config

@targos

targos Feb 1, 2016

Member

try to add spread: true to ecmaFeatures in our eslint config

This comment has been minimized.

@kittens

kittens Feb 1, 2016

@chrisdickinson Looks like the ecmaFeatures section of .eslintrc is missing spread: true.

@kittens

kittens Feb 1, 2016

@chrisdickinson Looks like the ecmaFeatures section of .eslintrc is missing spread: true.

This comment has been minimized.

@kittens

kittens Feb 1, 2016

@chrisdickinson Also I'd be worried that the use of arguments here causing a deoptimisation. It should be better to use a rest param and refer to that.

@kittens

kittens Feb 1, 2016

@chrisdickinson Also I'd be worried that the use of arguments here causing a deoptimisation. It should be better to use a rest param and refer to that.

This comment has been minimized.

@Fishrock123

Fishrock123 Feb 1, 2016

Member

Afaik rest params are currently still quite slow in our versions of V8, can we avoid them for now?

@Fishrock123

Fishrock123 Feb 1, 2016

Member

Afaik rest params are currently still quite slow in our versions of V8, can we avoid them for now?

@gergelyke

This comment has been minimized.

Show comment
Hide comment
@gergelyke

gergelyke Feb 1, 2016

Contributor

Hey,
just out of curiosity: why the need for setPromiseImplementation? Wouldn't it be better to go with the standard ES6 promises?

Contributor

gergelyke commented Feb 1, 2016

Hey,
just out of curiosity: why the need for setPromiseImplementation? Wouldn't it be better to go with the standard ES6 promises?

@chrisdickinson

This comment has been minimized.

Show comment
Hide comment
@chrisdickinson

chrisdickinson Feb 1, 2016

Contributor

@gergelyke This provides an escape hatch at application for folks that prefer different promise implementations, while also making sure that core doesn't suck the air out of the promise library ecosystem. Libraries can compete on features and performance without one implementation or the other winning by default. This also means that, in a vm-neutral future, developers working on applications can shield themselves from differences between VMs to some degree by using a third-party promises library.

Contributor

chrisdickinson commented Feb 1, 2016

@gergelyke This provides an escape hatch at application for folks that prefer different promise implementations, while also making sure that core doesn't suck the air out of the promise library ecosystem. Libraries can compete on features and performance without one implementation or the other winning by default. This also means that, in a vm-neutral future, developers working on applications can shield themselves from differences between VMs to some degree by using a third-party promises library.

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 1, 2016

Is process.setPromiseImplementation necessary? What does it achieve that global.Promise = foo; doesn't?

kittens commented Feb 1, 2016

Is process.setPromiseImplementation necessary? What does it achieve that global.Promise = foo; doesn't?

@chrisdickinson

This comment has been minimized.

Show comment
Hide comment
@chrisdickinson

chrisdickinson Feb 1, 2016

Contributor

@timoxley, @kittens: Mostly this is so we can use a "verified good for our purposes" promise implementation, and guard it with "hey, don't try and set the implementation twice" warnings. This also means that core always creates known-native promises at the outset and only casts them to user-chosen implementations. This may prove useful when combined with v8 dev tools (OTOH, it could be a case of YAGNI!)

Contributor

chrisdickinson commented Feb 1, 2016

@timoxley, @kittens: Mostly this is so we can use a "verified good for our purposes" promise implementation, and guard it with "hey, don't try and set the implementation twice" warnings. This also means that core always creates known-native promises at the outset and only casts them to user-chosen implementations. This may prove useful when combined with v8 dev tools (OTOH, it could be a case of YAGNI!)

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Feb 1, 2016

Member

@gergelyke Note that native v8 promises performance / memory consumption is far from perfect atm. See https://github.com/petkaantonov/bluebird/tree/master/benchmark for a comparison (note the difference between promises-ecmascript6-native and promises-bluebird.

Member

ChALkeR commented Feb 1, 2016

@gergelyke Note that native v8 promises performance / memory consumption is far from perfect atm. See https://github.com/petkaantonov/bluebird/tree/master/benchmark for a comparison (note the difference between promises-ecmascript6-native and promises-bluebird.

@madbence

This comment has been minimized.

Show comment
Hide comment
@madbence

madbence Feb 1, 2016

@timoxley the Promise impl should be controlled by the application you write. If a library tries to mess with global.Promise (or process.setPromiseImplementation), that's just wrong.

madbence commented Feb 1, 2016

@timoxley the Promise impl should be controlled by the application you write. If a library tries to mess with global.Promise (or process.setPromiseImplementation), that's just wrong.

@chrisdickinson

This comment has been minimized.

Show comment
Hide comment
@chrisdickinson

chrisdickinson Feb 1, 2016

Contributor

@madbence That's why setPromiseImplementation warns (with a trace noting both the original setPromiseImplementation call as well as a trace to the second call) on subsequent attempts to set the promise impl — so you can track down the offending module and remove it from your application. This API is intended for top-level applications only.

Contributor

chrisdickinson commented Feb 1, 2016

@madbence That's why setPromiseImplementation warns (with a trace noting both the original setPromiseImplementation call as well as a trace to the second call) on subsequent attempts to set the promise impl — so you can track down the offending module and remove it from your application. This API is intended for top-level applications only.

@timoxley

This comment has been minimized.

Show comment
Hide comment
@timoxley

timoxley Feb 1, 2016

Contributor

@chrisdickinson I guess this raises the question of why add this feature specifically for Promises? should node provide pluggable implementations for other APIs as well?

Contributor

timoxley commented Feb 1, 2016

@chrisdickinson I guess this raises the question of why add this feature specifically for Promises? should node provide pluggable implementations for other APIs as well?

@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Feb 1, 2016

Contributor

net.connectAsync() is fundamentally improperly named. The "executor" is still ran immediately, and if no hostname is given then TCP will bind to the port immediately. To get around this synchronous behavior node places the callback in a nextTick. Since I assume the callback will execute through the resolver it will be placed on the MicrotaskQueue and executed just after the nextTickQueue is completed. Causing another nextTick to be queued, and the nextTickQueue to be processed again.

In the end this API isn't any more async than it currently is.

Also I am curious how promisify is supposed to handle swallowing errors if the user's actual callback is fired on a different stack.

Contributor

trevnorris commented Feb 1, 2016

net.connectAsync() is fundamentally improperly named. The "executor" is still ran immediately, and if no hostname is given then TCP will bind to the port immediately. To get around this synchronous behavior node places the callback in a nextTick. Since I assume the callback will execute through the resolver it will be placed on the MicrotaskQueue and executed just after the nextTickQueue is completed. Causing another nextTick to be queued, and the nextTickQueue to be processed again.

In the end this API isn't any more async than it currently is.

Also I am curious how promisify is supposed to handle swallowing errors if the user's actual callback is fired on a different stack.

@chrisdickinson

This comment has been minimized.

Show comment
Hide comment
@chrisdickinson

chrisdickinson Feb 1, 2016

Contributor

@timoxley It's possible, though we should be cautious about applying this tactic elsewhere. I'd be interested in seeing an implementation that made streams pluggable, though that might fall down since streams are not as thoroughly specified as Promises.

Contributor

chrisdickinson commented Feb 1, 2016

@timoxley It's possible, though we should be cautious about applying this tactic elsewhere. I'd be interested in seeing an implementation that made streams pluggable, though that might fall down since streams are not as thoroughly specified as Promises.

@ehd

This comment has been minimized.

Show comment
Hide comment
@ehd

ehd Feb 1, 2016

I think React especially has shown the value of helpful runtime developer warnings. Having a function generate that warning sounds more sensible to me than a setter for example.

@timoxley It does, yet Core Promises have been waiting for their comeback for a long time. I would really like to see a more pluggable core, but it probably needs to be discussed on a case by case basis.

ehd commented Feb 1, 2016

I think React especially has shown the value of helpful runtime developer warnings. Having a function generate that warning sounds more sensible to me than a setter for example.

@timoxley It does, yet Core Promises have been waiting for their comeback for a long time. I would really like to see a more pluggable core, but it probably needs to be discussed on a case by case basis.

@chrisdickinson

This comment has been minimized.

Show comment
Hide comment
@chrisdickinson

chrisdickinson Feb 1, 2016

Contributor

@trevnorris:

net.connectAsync() is fundamentally improperly named.

The name is up for debate. I'm using this naming scheme to match what promise users are currently expecting, but I am open to changing it. Re: the nextTick behavior, that is an excellent point, and I will look into that tomorrow evening (if no one beats me to it.)

Contributor

chrisdickinson commented Feb 1, 2016

@trevnorris:

net.connectAsync() is fundamentally improperly named.

The name is up for debate. I'm using this naming scheme to match what promise users are currently expecting, but I am open to changing it. Re: the nextTick behavior, that is an excellent point, and I will look into that tomorrow evening (if no one beats me to it.)

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Feb 1, 2016

Member

What frightens me about the process.setPromiseImplementation API is that it could prevent us from switching to a creation of Promises in C++ code. Have you thought about it ?

Member

targos commented Feb 1, 2016

What frightens me about the process.setPromiseImplementation API is that it could prevent us from switching to a creation of Promises in C++ code. Have you thought about it ?

@vkurchatkin

This comment has been minimized.

Show comment
Hide comment
@vkurchatkin

vkurchatkin Feb 1, 2016

Member

-1 for any kind of support for non-native promises

Member

vkurchatkin commented Feb 1, 2016

-1 for any kind of support for non-native promises

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Feb 1, 2016

Member

Has this PR any effect on the performance of standard, callback driven methods?
Will this make things slower? Basically for any call to core, this adds one more function in the stack.
The idea of adding promises is compelling, but I fear wrapping might not be the solution.

Would you mind running the bench pre/post and putting those in a gist?

I think process.setPromiseImplementation() is inviting people to "monkey patch" node. I think this is a complete anti-pattern, and we should restrain to do so. Maybe even delaying this PR until V8 has a good native Promise implementation.

Member

mcollina commented Feb 1, 2016

Has this PR any effect on the performance of standard, callback driven methods?
Will this make things slower? Basically for any call to core, this adds one more function in the stack.
The idea of adding promises is compelling, but I fear wrapping might not be the solution.

Would you mind running the bench pre/post and putting those in a gist?

I think process.setPromiseImplementation() is inviting people to "monkey patch" node. I think this is a complete anti-pattern, and we should restrain to do so. Maybe even delaying this PR until V8 has a good native Promise implementation.

@chrisdickinson

This comment has been minimized.

Show comment
Hide comment
@chrisdickinson

chrisdickinson Feb 1, 2016

Contributor

@targos Yep — with this implementation, Node always creates native promises. lib/internal/promises goes as far as to ensure that it runs before any other user code can possibly run so that it can grab a reference to the "authentic" Promise constructor for other modules to use. The implementation only casts native promises to user-selected implementations, instead of outright constructing them — this way the C++ layer can continue to make native promises.

@vkurchatkin I'd be interested to hear why. It seems like a fairly low cost to us to support this, while giving application authors the freedom to standardize Node on the implementation they're already using, letting the promise library ecosystem thrive and giving them more benchmarks to compete on, giving end users a way to shield against quirks across VMs, and letting users opt into faster implementations where desired. It does add complexity, and probably more worrying, adds another application level API. The complexity is mostly encompassed by "we run our promise through a user's Promise.resolve", though, so it doesn't seem too bad. I've tried to mitigate the application-level API problem by adding trackable warnings — does this not go far enough?

Contributor

chrisdickinson commented Feb 1, 2016

@targos Yep — with this implementation, Node always creates native promises. lib/internal/promises goes as far as to ensure that it runs before any other user code can possibly run so that it can grab a reference to the "authentic" Promise constructor for other modules to use. The implementation only casts native promises to user-selected implementations, instead of outright constructing them — this way the C++ layer can continue to make native promises.

@vkurchatkin I'd be interested to hear why. It seems like a fairly low cost to us to support this, while giving application authors the freedom to standardize Node on the implementation they're already using, letting the promise library ecosystem thrive and giving them more benchmarks to compete on, giving end users a way to shield against quirks across VMs, and letting users opt into faster implementations where desired. It does add complexity, and probably more worrying, adds another application level API. The complexity is mostly encompassed by "we run our promise through a user's Promise.resolve", though, so it doesn't seem too bad. I've tried to mitigate the application-level API problem by adding trackable warnings — does this not go far enough?

@eljefedelrodeodeljefe

This comment has been minimized.

Show comment
Hide comment
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Feb 1, 2016

Contributor

I am hugely against promises whatsoever. However could the promisification be an explicit opt-in process.promisify(true) // conditional export in libs instead of affecting any function there is?

Contributor

eljefedelrodeodeljefe commented Feb 1, 2016

I am hugely against promises whatsoever. However could the promisification be an explicit opt-in process.promisify(true) // conditional export in libs instead of affecting any function there is?

+ function inner() {
+ const cb = arguments[arguments.length - 1];
+ if (typeof cb !== 'function') {
+ cb = (err) => { throw err; };

This comment has been minimized.

@reqshark

reqshark Feb 1, 2016

why not do this:

cb = err => throw err;
@reqshark

reqshark Feb 1, 2016

why not do this:

cb = err => throw err;

This comment has been minimized.

@madbence

madbence Feb 1, 2016

becase that's a SyntaxError. in the () => expr syntax, the expr should be an expression. the parens around err can be removed (but i guess there's a rule in the linter for that)

@madbence

madbence Feb 1, 2016

becase that's a SyntaxError. in the () => expr syntax, the expr should be an expression. the parens around err can be removed (but i guess there's a rule in the linter for that)

This comment has been minimized.

@chrisdickinson

chrisdickinson Feb 1, 2016

Contributor

Ah, only expressions are valid bodies for braceless arrow functions — err => throw err doesn't parse.

@chrisdickinson

chrisdickinson Feb 1, 2016

Contributor

Ah, only expressions are valid bodies for braceless arrow functions — err => throw err doesn't parse.

This comment has been minimized.

@timoxley

timoxley Feb 1, 2016

Contributor

@reqshark can't throw in an expression. Without curlies, RHS of arrow is an expression

@timoxley

timoxley Feb 1, 2016

Contributor

@reqshark can't throw in an expression. Without curlies, RHS of arrow is an expression

This comment has been minimized.

@reqshark

reqshark Feb 1, 2016

yea I think what I did was SyntaxError: Unexpected token throw, you're right

but the parens stuff I disagree, for example i think this is ok:

cb = err => { throw err; };

the brackets prevent the syntax err IMO, not the parenthesis

@reqshark

reqshark Feb 1, 2016

yea I think what I did was SyntaxError: Unexpected token throw, you're right

but the parens stuff I disagree, for example i think this is ok:

cb = err => { throw err; };

the brackets prevent the syntax err IMO, not the parenthesis

This comment has been minimized.

@timoxley

timoxley Feb 1, 2016

Contributor

@reqshark Parens required by linter #4813

@timoxley

timoxley Feb 1, 2016

Contributor

@reqshark Parens required by linter #4813

@vkurchatkin

This comment has been minimized.

Show comment
Hide comment
@vkurchatkin

vkurchatkin Feb 1, 2016

Member

@chrisdickinson the only reason we really consider including promises in core is that they are a part of language and a part of v8. It's not a good idea to include API that we can be sure will become obsolete and could potentially prevent all kinds of optimisations in the nearest future. If we think that v8 promises are not good enough - we shouldn't do it at all.

Member

vkurchatkin commented Feb 1, 2016

@chrisdickinson the only reason we really consider including promises in core is that they are a part of language and a part of v8. It's not a good idea to include API that we can be sure will become obsolete and could potentially prevent all kinds of optimisations in the nearest future. If we think that v8 promises are not good enough - we shouldn't do it at all.

+
+ function resolver(arg0, arg1, arg2) {
+ try {
+ resolve(handler(lastArg, arg0, arg1, arg2));

This comment has been minimized.

@jfhbrook

jfhbrook Feb 15, 2016

Contributor

This reminds me of cases where I do something like cb(null, JSON.parse(blob)) and the cb throws rather than the JSON.parse and I get confused and angry. This might not be an analogous case (would you ever expect resolve to throw ever?) but I figured I'd make note of it.

@jfhbrook

jfhbrook Feb 15, 2016

Contributor

This reminds me of cases where I do something like cb(null, JSON.parse(blob)) and the cb throws rather than the JSON.parse and I get confused and angry. This might not be an analogous case (would you ever expect resolve to throw ever?) but I figured I'd make note of it.

This comment has been minimized.

@benjamingr

benjamingr Feb 15, 2016

Member

@jfhbrook yes, promises normalize that - if you pass JSON.parse to a promise or async method you'll get consistent behavior somePromiseFn().then(JSON.parse) does not result in a synchronous throw - but rather an asynchronous rejection. If you don't handle it you get an unhandledRejection but it is recoverable. This also has the benefit of catching any places you forgot to deal the error case. For example, with promises you don't have to explicitly type if(err) return cb(err); (and forget the return :P). With promises every error path is checked and propagated - like in completely synchronous code.

@benjamingr

benjamingr Feb 15, 2016

Member

@jfhbrook yes, promises normalize that - if you pass JSON.parse to a promise or async method you'll get consistent behavior somePromiseFn().then(JSON.parse) does not result in a synchronous throw - but rather an asynchronous rejection. If you don't handle it you get an unhandledRejection but it is recoverable. This also has the benefit of catching any places you forgot to deal the error case. For example, with promises you don't have to explicitly type if(err) return cb(err); (and forget the return :P). With promises every error path is checked and propagated - like in completely synchronous code.

@pesho

This comment has been minimized.

Show comment
Hide comment
@pesho

pesho Feb 15, 2016

Member

@chrisdickinson

With regards to a require('fs/promise') API, if this PR is landed I will immediately follow up with a PR proposing a submodule approach so we can have that discussion. Your concerns that the existing approach does not work well with ES2015 modules are well-founded and I'd like to start addressing that as soon we get an idea of how we want to move forward with this API. We can work towards making the submodule approach the official approach to using the promise API before unflagging.

Why provide several interfaces, instead of just picking the best one? The Perl ideology ("There's more than one way to do it") has been judged wrong by history.

To me require('fs/promise') is much more preferable than require('fs').promised and I'd rather skip the second one completely. This has several benefits:

  • ES6-style imports: import { readFile } from 'fs/promise'; vs ???.
  • require('fs') will not load promisified variants, saving memory (for people who don't care about promises).
  • The changes to the existing modules will be minimal, thus minimizing the need for consensus-seeking.
Member

pesho commented Feb 15, 2016

@chrisdickinson

With regards to a require('fs/promise') API, if this PR is landed I will immediately follow up with a PR proposing a submodule approach so we can have that discussion. Your concerns that the existing approach does not work well with ES2015 modules are well-founded and I'd like to start addressing that as soon we get an idea of how we want to move forward with this API. We can work towards making the submodule approach the official approach to using the promise API before unflagging.

Why provide several interfaces, instead of just picking the best one? The Perl ideology ("There's more than one way to do it") has been judged wrong by history.

To me require('fs/promise') is much more preferable than require('fs').promised and I'd rather skip the second one completely. This has several benefits:

  • ES6-style imports: import { readFile } from 'fs/promise'; vs ???.
  • require('fs') will not load promisified variants, saving memory (for people who don't care about promises).
  • The changes to the existing modules will be minimal, thus minimizing the need for consensus-seeking.
@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Feb 15, 2016

Member

@pesho
To address ???, it'd be:

import {promised: fs } from `fs`
// or only a single function
import {promised: {readFile} } from `fs`

Which would let you do readFile with promises easily.

Member

benjamingr commented Feb 15, 2016

@pesho
To address ???, it'd be:

import {promised: fs } from `fs`
// or only a single function
import {promised: {readFile} } from `fs`

Which would let you do readFile with promises easily.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Feb 15, 2016

Member

@benjamingr I don't think you can use destructuring in import statements.
The first line has to be written as import {promised as fs} from 'fs'
The second is not possible in one line AFAIK.

Member

targos commented Feb 15, 2016

@benjamingr I don't think you can use destructuring in import statements.
The first line has to be written as import {promised as fs} from 'fs'
The second is not possible in one line AFAIK.

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens Feb 15, 2016

@targos is correct. The promised property is not going to be ergonomic for promise users due to this. An alternate module source would be best like the suggested fs/promise.

kittens commented Feb 15, 2016

@targos is correct. The promised property is not going to be ergonomic for promise users due to this. An alternate module source would be best like the suggested fs/promise.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Feb 15, 2016

Member

Oh yeah, that's definitely my bad @targos - indeed it would require a second line. I never had to do it and I guess I just assumed it'd work.

Member

benjamingr commented Feb 15, 2016

Oh yeah, that's definitely my bad @targos - indeed it would require a second line. I never had to do it and I guess I just assumed it'd work.

@benjamingr benjamingr referenced this pull request in nodejs/promises Feb 15, 2016

Closed

Promises Working Group, Call For Contributors #1

@chrisdickinson

This comment has been minimized.

Show comment
Hide comment
@chrisdickinson

chrisdickinson Feb 15, 2016

Contributor

@pesho, @kittens, @benjamingr: Yep — I'll be following up this PR with a PR that creates a require('fs/promise') module just for that use case. If we can get consensus on that before unflagging promises the promise API, that will be the official way to access top-level Promise APIs.

Contributor

chrisdickinson commented Feb 15, 2016

@pesho, @kittens, @benjamingr: Yep — I'll be following up this PR with a PR that creates a require('fs/promise') module just for that use case. If we can get consensus on that before unflagging promises the promise API, that will be the official way to access top-level Promise APIs.

@balupton balupton referenced this pull request in nodejs/promises Feb 16, 2016

Open

How promise APIs would look like in core #16

@balupton

This comment has been minimized.

Show comment
Hide comment
@balupton

balupton Feb 16, 2016

As no one has mentioned it yet. Discussion of this PR has been split up into individual discussions on https://github.com/nodejs/promises to improve digestion - discussion will likely continue here for things related specifically to this PR only, and using https://github.com/nodejs/promises for commenting on any topics that are already over there (see its issue tracker).

As no one has mentioned it yet. Discussion of this PR has been split up into individual discussions on https://github.com/nodejs/promises to improve digestion - discussion will likely continue here for things related specifically to this PR only, and using https://github.com/nodejs/promises for commenting on any topics that are already over there (see its issue tracker).

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 16, 2016

Member

One other point. Please keep any further discussion on this PR focused on the technical review of this PR. The proper venue to discuss the more meta level issues of promises in core is the new promises repo. Comments in this PR that are off topic can and likely will be moderated. It's also possible that we may lock the thread for non collaborators. We don't like doing that and don't want to do that so please keep the conversation focused and on topic.

Member

jasnell commented Feb 16, 2016

One other point. Please keep any further discussion on this PR focused on the technical review of this PR. The proper venue to discuss the more meta level issues of promises in core is the new promises repo. Comments in this PR that are off topic can and likely will be moderated. It's also possible that we may lock the thread for non collaborators. We don't like doing that and don't want to do that so please keep the conversation focused and on topic.

@matthewarkin matthewarkin referenced this pull request in stripe/stripe-node Mar 23, 2016

Open

Automatic pagination with .list functions #225

@brianleroux brianleroux referenced this pull request in Azure/azure-functions-host Apr 1, 2016

Closed

Allow returning promises from functions as opposed to context.done() #158

@zkat zkat referenced this pull request in nodejs/inclusivity Apr 18, 2016

Closed

Continuous translation threads for important discussions #102

@disjukr disjukr referenced this pull request in libreirc/qbirc Apr 24, 2016

Open

API using Promise #12

@yosuke-furukawa yosuke-furukawa referenced this pull request in nodejs/promises Apr 30, 2016

Closed

Default Unhandled Rejection Behavior #26

@saadq saadq referenced this pull request in nodejs/help May 1, 2016

Closed

Question about promises #158

@forivall forivall referenced this pull request in amacneil/fetch-test-server May 17, 2016

Closed

Allow setting a custom promise implementation #3

@syrnick syrnick referenced this pull request in nodejs/node-v0.x-archive May 19, 2016

Open

promises prevent domain propagation #8648

@chrisdickinson chrisdickinson removed their assignment May 28, 2016

@benjamingr benjamingr referenced this pull request in nodejs/CTC Jun 9, 2016

Closed

`async`/`await` and Node #7

@fhemberger fhemberger referenced this pull request in nodejs/nodejs.org Sep 25, 2016

Closed

Abort trap 6. when dns servers are invalid #918

@danbucholtz

This comment has been minimized.

Show comment
Hide comment
@danbucholtz

danbucholtz Oct 26, 2016

@chrisdickinson, there's a lot to take in here 😄 . Is there an update on the general topic of Node.js core API supporting promises instead of callbacks?

Thanks,
Dan

@chrisdickinson, there's a lot to take in here 😄 . Is there an update on the general topic of Node.js core API supporting promises instead of callbacks?

Thanks,
Dan

@reconbot reconbot referenced this pull request in node-serialport/node-serialport Nov 6, 2016

Open

Explore Promises in addition to callbacks #991

@smashwilson smashwilson referenced this pull request in atom/github Feb 2, 2017

Merged

Discard lines in file diff #487

3 of 3 tasks complete

@azu azu referenced this pull request in asciidwango/js-primer Feb 24, 2017

Closed

2017-02-24 ミーティングアジェンダ #191

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 28, 2017

Member

Closing given the lack of forward progress on this. We'll definitely want to revisit this later tho.

Member

jasnell commented Feb 28, 2017

Closing given the lack of forward progress on this. We'll definitely want to revisit this later tho.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment