util: add util.promisify() #12442

addaleax · 2017-04-16T16:30:05Z

See nodejs/CTC#12 for discussion/background. I would say that that thread is a better place for general discussion, and I prefer it if comments here would be kept to what is directly relevant for this PR.

CI: https://ci.nodejs.org/job/node-test-commit/9143/

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

util, timers

Original commit descriptions

util: add internal bindings for promise handling

Add methods for creating, resolving and rejecting promises using the V8 C++ API that does not require creation of extra resolve and reject functions to process.binding('util').

util: add util.promisify()

Add util.promisify(function) for creating promisified functions.

Fixes: nodejs/CTC#12

timers: add promisify support

Add support for util.promisify(setTimeout) and util.promisify(setImmediate) as a proof-of-concept implementation. ~~clearTimeout() and clearImmediate() resolve the promise immediately instead of rejecting it; that might be the most opinionated choice about this.~~

/cc @chrisdickinson @benjamingr @Fishrock123 @nodejs/ctc

edit: CTC voting comment

addaleax · 2017-04-16T16:30:21Z

Also, @benjamingr has a good point about the naming, quote:

optimally I'd hope for most people to not even be aware of what a promise is in 5 years and just use async/await seamlessly :)

We can bikeshed this further here, but just to get an idea how people feel, could people 👍 this comment for “promisify sounds good” and 👎 this for “I’d prefer awaitable or something else” (both assuming that we agree on the approach in this PR in general)?

Fishrock123 · 2017-04-16T16:56:49Z

Maybe we should split the timers bits into a second PR? They don't map well to promises since we don't have cancelables. Maybe we could plug the timers props onto the promise itself? Not sure.

Fishrock123 · 2017-04-16T16:58:33Z

I.e. clearTimeout() should probably work on whatever setTimeoutPromise() returns (a promise but you get the idea), same for Immediates.

williamkapke

I (also) would like to see the timers impl in a followup PR.

williamkapke · 2017-04-16T16:56:38Z

lib/internal/util.js

+    throw new TypeError('The first argument to promisify() must be a function');
+  }
+
+  if (orig && orig[kCustomPromisifiedSymbol]) {


This truthy test of orig is not needed since it is guaranteed to be a function at this point

@williamkapke thanks for catching, done

benjamingr · 2017-04-16T17:10:26Z

A big +1 on this. I think this could be a great first step for helping users deal with async/await in their code. There is a clear benefit over a userland implementation, and the need is prevalent.

Fishrock123 · 2017-04-16T17:11:39Z

Going to expand on my point a bit: I think this is reasonable to do for simple callback APIs but I think we should spend more* time to think about how to wrap more complex APIs such as timers, child processes, etc well.

* ~~Might be good for an EP?~~

addaleax · 2017-04-16T17:12:09Z

Maybe we should split the timers bits into a second PR?

If that’s what people here prefer, sure. I just thought it made sense because they’re about the only API that’s a bit icky because they don’t conform to Node’s standard callback pattern, so whenever we introduce something like util.promisify() we’d probably also want to have support for promisified timers (in some way).

I.e. clearTimeout() should probably work on whatever setTimeoutPromise() returns (a promise but you get the idea), same for Immediates.

Makes sense, I’ve updated the PR with isPromise(timer) checks anyway.

benjamingr · 2017-04-16T17:17:32Z

@Fishrock123

Going to expand on my point a bit: I think this is reasonable to do for simple callback APIs but I think we should spend more time to think about how to wrap more complex APIs such as timers, child processes, etc well.

I definitely agree - we need a proper promised core at some point - but #5020 and similar attempts have stagnated and quoting myself in the original issue:

The idea of util.awaitable vs a promise core is that it's hopefully a lot less objectionable, it poses a lot less controversy, it's a lot less opinionated and it means exposing a capability that users can build on and not an API.

addaleax · 2017-04-16T17:20:06Z

I think @Fishrock123’s comment was about the timers stuff, not the general API here? I feel inclined to disagree that this needs an EP, though…

Fishrock123 · 2017-04-16T17:21:02Z

Correct. I only mean for how to wrap any APIs that are more complex than single events or callbacks and which do not wrap as well.

TimothyGu · 2017-04-16T17:09:50Z

doc/api/util.md

+added: REPLACEME
+-->
+
+* `function` {Function}


function isn't a legal identifier in JS, and for consistency we probably want to keep it legal.

TimothyGu · 2017-04-16T17:10:18Z

doc/api/util.md

+<!-- YAML
+added: REPLACEME
+-->
+


TimothyGu · 2017-04-16T17:10:32Z

doc/api/util.md

+-->
+
+A Symbol that can be used to declare custom promisified variants of functions,
+see [Custom promisified functions][].


Lower case "custom"

I think the uppercase variant is okay here, it’s referring to a literal section title, and it matches what we do for the other two section references in this file. If you feel strongly about it I’ll change it.

TimothyGu · 2017-04-16T17:12:59Z

doc/api/timers.md

+setImmediatePromise('foobar').then((value) => {
+  // value === 'foobar' (passing values is optional)
+  // This is executed after all I/O callbacks.
+});


An async function example would be cool as well.

TimothyGu · 2017-04-16T17:21:39Z

doc/api/timers.md

@@ -137,7 +169,7 @@ cancel the timer and prevent it from triggering.
 added: v0.9.1
 -->

-* `immediate` {Immediate} An `Immediate` object as returned by
+* `immediate` {Immediate|Promise} An `Immediate` object as returned by
  [`setImmediate()`][].

 Cancels an `Immediate` object created by [`setImmediate()`][].


You probably want to clarify setImmediate() or its promisified equivalent.

TimothyGu · 2017-04-16T17:21:47Z

doc/api/timers.md

@@ -156,7 +188,7 @@ Cancels a `Timeout` object created by [`setInterval()`][].
 added: v0.0.1
 -->

-* `timeout` {Timeout} A `Timeout` object as returned by [`setTimeout()`][].
+* `timeout` {Timeout|Promise} A `Timeout` object as returned by [`setTimeout()`][].

 Cancels a `Timeout` object created by [`setTimeout()`][].


TimothyGu · 2017-04-16T17:22:42Z

doc/api/util.md

+
+### Custom promisified functions
+
+Using the `util.promisify.custom` one can override the return value of


"Using the util.promisify.custom symbol"?

TimothyGu · 2017-04-16T17:23:24Z

lib/internal/util.js

+    return promise;
+  };
+
+  return Object.defineProperties(fn, Object.getOwnPropertyDescriptors(orig));


What about the prototype of orig?

@TimothyGu yeah … that thought occurred to me, but I basically didn’t see how setting it would make sense here. If you prefer, I’ll add

Object.setPrototypeOf(fn, Object.getPrototypeOf(orig));

Yeah I don't see how it would matter that much in real world, but having that there does seem a bit more natural to me. Up to you if you want to set it or not.

I don't think it would matter that much; it just occurred to me having the prototype being equivalent might be more natural. Up to you.

@TimothyGu I’ve included it for now, let’s see what people think.

addaleax · 2017-04-16T17:41:10Z

@TimothyGu Addressed most of your comments, PTAL

vkurchatkin · 2017-04-16T17:57:38Z

lib/timers.js

    timer[kOnTimeout] = timer._onTimeout = null;
    if (timer instanceof Timeout) {
      timer.close(); // for after === 0
    } else {
      unenroll(timer);
    }
+  } else if (isPromise(timer)) {


This seems like a bad idea for many reasons. I think it's fine if you can't cancel promisified timeout until we have some generic cancellation mechanism.

This seems like a bad idea for many reasons.

Would you care to provide some? Otherwise this is just not helpful.

First of all, attaching properties to native objects seems like a bad idea from performance perspective. Secondly, this implements ad hoc cancellation mechanism, while there is no spec on standards track or even community consensus. This approach suffers from the fact that it's not propagated and there it doesn't play nice with async-await.

I think it's fine to make promisified versions not cancellable. If you use them you are most likely not going to cancel anyway.

First of all, attaching properties to native objects seems like a bad idea from performance perspective.

That’s true, but it’s opt-in, right?

This approach suffers from the fact that it's not propagated

What do you mean by propagated?

But yeah, I would be okay with dropping clear* support for these, at least for the time being.

What do you mean by propagated?

Something like this:

const promise = setTimeout(1000) .then(() => ...); clearTimeout(promise); // This won't work, you need original promise

I have to agree with @vkurchatkin on this. Until there is a standard way of canceling promises, we likely shouldn't do this.

@vkurchatkin @jasnell Okay, I’ve dropped clearTimeout/clearImmediate support. It’s a niche feature anyway and it should be easy to readjust later if we want.

Add `util.promisify(function)` for creating promisified functions. Includes documentation and tests. Fixes: nodejs/CTC#12 PR-URL: nodejs#12442 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: William Kapke <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

Take tests from Bluebird's promisify's tests and adapted them to the format in use here. Add tests making sure things work with async functions. Add basic usability tests. PR-URL: nodejs#12442 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: William Kapke <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

PR-URL: nodejs#12442 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: William Kapke <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

Add support for `util.promisify(setTimeout)` and `util.promisify(setImmediate)` as a proof-of-concept implementation. `clearTimeout()` and `clearImmediate()` do not work on those Promises. Includes documentation and tests. PR-URL: nodejs#12442 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: William Kapke <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

PR-URL: nodejs#12442 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: William Kapke <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

Author: Benjamin Gruenbaum <[email protected]> Author: Anna Henningsen <[email protected]> PR-URL: nodejs#12442 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: William Kapke <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

PR-URL: nodejs#12442 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: William Kapke <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](nodejs@4a7233c178)] [nodejs#12892](nodejs#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](nodejs@d2d32ea5a2)] [nodejs#11968](nodejs#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](nodejs@7eb1b4658e)] [nodejs#12141](nodejs#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](nodejs@beca3244e2)] [nodejs#10236](nodejs#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](nodejs@97a77288ce)] [nodejs#12348](nodejs#12348), [[`d75fdd96aa`](nodejs@d75fdd96aa)] [nodejs#10423](nodejs#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](nodejs@627ecee9ed)] [nodejs#10653](nodejs#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](nodejs@f18e08d820)] [nodejs#9744](nodejs#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](nodejs@3c3b36af0f)] [nodejs#12936](nodejs#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](nodejs@60d1aac8d2)] [nodejs#12784](nodejs#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](nodejs@84dabe8373)] [nodejs#12489](nodejs#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](nodejs@7a55e34ef4)] [nodejs#10467](nodejs#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](nodejs@3c2a9361ff)] [nodejs#9683](nodejs#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](nodejs@90403dd1d0)] [nodejs#11567](nodejs#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](nodejs@d3480776c7)] [nodejs#11259](nodejs#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](nodejs@fb71ba4921)] [nodejs#11355](nodejs#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](nodejs@3e6f1032a4)] [nodejs#10805](nodejs#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](nodejs@5de3cf099c)] [nodejs#10116](nodejs#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](nodejs@84a23391f6)] [nodejs#12113](nodejs#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](nodejs@56e881d0b0)] [nodejs#11975](nodejs#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](nodejs@03e89b3ff2)] [nodejs#10116](nodejs#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](nodejs@dd20e68b0f)] [nodejs#12725](nodejs#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](nodejs@3f27f02da0)] [nodejs#11599](nodejs#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (nodejs@ec7cbaf266)] [nodejs#12995](nodejs#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](nodejs@a16b570f8c)] [nodejs#11968](nodejs#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](nodejs@010f864426)] [nodejs#12949](nodejs#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](nodejs@a5f91ab230)] [nodejs#11689](nodejs#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](nodejs@8a7db9d4b5)] [nodejs#12087](nodejs#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](nodejs@b6e1d22fa6)] [nodejs#12925](nodejs#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](nodejs@07c7f198db)] [nodejs#12828](nodejs#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](nodejs@348cc80a3c)] [nodejs#5923](nodejs#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](nodejs@a2ae08999b)] [nodejs#11349](nodejs#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](nodejs@d523eb9c40)] [nodejs#11447](nodejs#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](nodejs@d080ead0f9)] [nodejs#12710](nodejs#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](nodejs@5bfd13b81e)] [nodejs#9726](nodejs#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](nodejs@455e6f1dd8)] [nodejs#11708](nodejs#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](nodejs@aab0d202f8)] [nodejs#11624](nodejs#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](nodejs@99da8e8e02)] [nodejs#12442](nodejs#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](nodejs@91383e47fd)] [nodejs#12001](nodejs#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](nodejs@b514bd231e)] [nodejs#11391](nodejs#11391).

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](4a7233c178)] [#12892](#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](d2d32ea5a2)] [#11968](#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](7eb1b4658e)] [#12141](#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](beca3244e2)] [#10236](#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](97a77288ce)] [#12348](#12348), [[`d75fdd96aa`](d75fdd96aa)] [#10423](#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](627ecee9ed)] [#10653](#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](f18e08d820)] [#9744](#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](3c3b36af0f)] [#12936](#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](60d1aac8d2)] [#12784](#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](84dabe8373)] [#12489](#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](7a55e34ef4)] [#10467](#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](3c2a9361ff)] [#9683](#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](90403dd1d0)] [#11567](#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](d3480776c7)] [#11259](#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](fb71ba4921)] [#11355](#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](3e6f1032a4)] [#10805](#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](5de3cf099c)] [#10116](#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](84a23391f6)] [#12113](#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](56e881d0b0)] [#11975](#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](03e89b3ff2)] [#10116](#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](dd20e68b0f)] [#12725](#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](3f27f02da0)] [#11599](#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (ec7cbaf266)] [#12995](#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](a16b570f8c)] [#11968](#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](010f864426)] [#12949](#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](a5f91ab230)] [#11689](#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](8a7db9d4b5)] [#12087](#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](b6e1d22fa6)] [#12925](#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](07c7f198db)] [#12828](#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](348cc80a3c)] [#5923](#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](a2ae08999b)] [#11349](#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](d523eb9c40)] [#11447](#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](d080ead0f9)] [#12710](#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](5bfd13b81e)] [#9726](#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](455e6f1dd8)] [#11708](#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](aab0d202f8)] [#11624](#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](99da8e8e02)] [#12442](#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](91383e47fd)] [#12001](#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](b514bd231e)] [#11391](#11391).

ljharb · 2017-05-30T21:23:21Z

I just published https://www.npmjs.com/package/util.promisify (with code largely copy/pasted from this PR, with permission from @addaleax) for those interested.

Ref: nodejs#12442 PR-URL: nodejs#12489 Backport-PR-URL: nodejs#13103 Reviewed-By: Matthew Loring <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>

Ref: #12442 PR-URL: #12489 Backport-PR-URL: #13103 Reviewed-By: Matthew Loring <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>

syrnick · 2017-07-18T16:23:59Z

lib/internal/util.js

+          promiseResolve(promise, values[0]);
+        }
+      });
+    } catch (err) {


This catch may accidentally catch errors thrown from promiseReject if orig.call happened to be synchronous. E.g. the value was cached in memory and didn't need a remote lookup. promiseReject on line 264 will be a no-op in that case.

Ideally if the execution hit line 253 and then 264, it should re-throw the error instead of no-oping. At least it could log a warning.

The existing way is definitely better; a function that returns a promise should never throw, only return a rejected promise.

@syrnick can you please open an issue about it?

(also, since a promise can only represent one value; new Promise((resolve, reject) => { reject(1); reject(2); }) is supposed to have the second rejection be a noop, and does not throw the 2, so it'd be important for util.promisify to maintain parity)

syrnick · 2018-01-31T01:18:40Z

lib/internal/util.js

@@ -217,3 +219,62 @@ module.exports = exports = {
  // default isEncoding implementation, just in case userland overrides it.
  kIsEncodingSymbol: Symbol('node.isEncoding')
 };
+
+const kCustomPromisifiedSymbol = Symbol('util.promisify.custom');
+const kCustomPromisifyArgsSymbol = Symbol('customPromisifyArgs');


What's the motivation for keeping this private? It's obviously useful in a number of places.

https://github.com/nodejs/node/pull/12442/files#diff-a43208147d795be6dd3517c53226e37dR280 ?

but that's still on internal/util, not accessible to user code. The other one(kCustomPromisifiedSymbol) is available as util.promisify.custom.

@syrnick See #14007.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. util Issues and PRs related to the built-in util module. labels Apr 16, 2017

addaleax mentioned this pull request Apr 16, 2017

Support awaiting values through util.awaitable nodejs/CTC#12

Closed

williamkapke suggested changes Apr 16, 2017

View reviewed changes

benjamingr self-assigned this Apr 16, 2017

addaleax force-pushed the promisify branch from c1f7c31 to 162f3c0 Compare April 16, 2017 17:11

addaleax force-pushed the promisify branch from 162f3c0 to 506b333 Compare April 16, 2017 17:11

williamkapke approved these changes Apr 16, 2017

View reviewed changes

TimothyGu reviewed Apr 16, 2017

View reviewed changes

addaleax force-pushed the promisify branch 3 times, most recently from ad34f91 to fe27d6a Compare April 16, 2017 17:40

vkurchatkin reviewed Apr 16, 2017

View reviewed changes

addaleax force-pushed the promisify branch from fe27d6a to 9d08d10 Compare April 16, 2017 17:58

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

divyenduz mentioned this pull request Jun 16, 2017

Support Promise redis/node-redis#864

Closed

TimothyGu mentioned this pull request Jun 30, 2017

Expose reference to the Symbol(customPromisifyArgs) #14007

Closed

syrnick reviewed Jul 18, 2017

View reviewed changes

benjamingr mentioned this pull request Aug 16, 2017

Promise Performance Meeting nodejs/promises#31

Open

benjamingr mentioned this pull request Sep 1, 2017

Allow passing objects to util.promisify nodejs/CTC#174

Closed

TimothyGu mentioned this pull request Sep 17, 2017

meta: planning for promisified fs and crypto #15413

Closed

merisbahti mentioned this pull request Oct 19, 2017

Just a comment merisbahti/promiser.js#1

Open

syrnick reviewed Jan 31, 2018

View reviewed changes

pi0 mentioned this pull request Oct 15, 2018

feat: use util.promisify when possible sindresorhus/pify#68

Closed

P-Seebauer mentioned this pull request Mar 24, 2019

util.callbackify return value problematic characteristics #26890

Closed


		### Custom promisified functions

		Using the `util.promisify.custom` one can override the return value of

util: add util.promisify() #12442

util: add util.promisify() #12442

Conversation

addaleax commented Apr 16, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Original commit descriptions

util: add internal bindings for promise handling

util: add util.promisify()

timers: add promisify support

addaleax commented Apr 16, 2017

Fishrock123 commented Apr 16, 2017

Fishrock123 commented Apr 16, 2017

williamkapke left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr commented Apr 16, 2017

Fishrock123 commented Apr 16, 2017 • edited Loading

addaleax commented Apr 16, 2017

benjamingr commented Apr 16, 2017

addaleax commented Apr 16, 2017

Fishrock123 commented Apr 16, 2017

TimothyGu Apr 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu Apr 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Apr 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented May 30, 2017

syrnick Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

ljharb Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Apr 16, 2017 •

edited

Loading

williamkapke left a comment •

edited

Loading

Fishrock123 commented Apr 16, 2017 •

edited

Loading

TimothyGu Apr 16, 2017 •

edited

Loading

TimothyGu Apr 16, 2017 •

edited

Loading

syrnick Jul 18, 2017 •

edited

Loading

ljharb Jul 18, 2017 •

edited

Loading