Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: add util.promisify() #12442

Merged
merged 8 commits into from
May 9, 2017
Merged

util: add util.promisify() #12442

merged 8 commits into from
May 9, 2017

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Apr 16, 2017

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 addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. promises Issues and PRs related to ECMAScript promises. semver-minor PRs that contain new features and should be released in the next minor version. 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
@nodejs-github-bot 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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Contributor

@williamkapke williamkapke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throw new TypeError('The first argument to promisify() must be a function');
}

if (orig && orig[kCustomPromisifiedSymbol]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@williamkapke thanks for catching, done

@benjamingr benjamingr self-assigned this Apr 16, 2017
@benjamingr
Copy link
Member

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
Copy link
Contributor

Fishrock123 commented Apr 16, 2017

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
Copy link
Member Author

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
Copy link
Member

@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
Copy link
Member Author

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
Copy link
Contributor

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.

doc/api/util.md Outdated
added: REPLACEME
-->

* `function` {Function}
Copy link
Member

@TimothyGu TimothyGu Apr 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<!-- YAML
added: REPLACEME
-->

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* {symbol}

doc/api/util.md Outdated
-->

A Symbol that can be used to declare custom promisified variants of functions,
see [Custom promisified functions][].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lower case "custom"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An async function example would be cool as well.

@@ -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()`][].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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()`][].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

doc/api/util.md Outdated

### Custom promisified functions

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Using the util.promisify.custom symbol"?

return promise;
};

return Object.defineProperties(fn, Object.getOwnPropertyDescriptors(orig));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the prototype of orig?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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));

Copy link
Member

@TimothyGu TimothyGu Apr 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@TimothyGu Addressed most of your comments, PTAL

lib/timers.js Outdated
timer[kOnTimeout] = timer._onTimeout = null;
if (timer instanceof Timeout) {
timer.close(); // for after === 0
} else {
unenroll(timer);
}
} else if (isPromise(timer)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a bad idea for many reasons.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by propagated?

Something like this:

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
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]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
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]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
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]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
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]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
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]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
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]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
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]>
@jasnell jasnell mentioned this pull request May 11, 2017
jasnell added a commit to jasnell/node that referenced this pull request May 29, 2017
* **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).
jasnell added a commit that referenced this pull request May 30, 2017
* **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).
jasnell added a commit that referenced this pull request May 30, 2017
* **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
Copy link
Member

ljharb commented May 30, 2017

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.

gibfahn pushed a commit to gibfahn/node that referenced this pull request Jun 17, 2017
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]>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
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]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
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]>
promiseResolve(promise, values[0]);
}
});
} catch (err) {
Copy link

@syrnick syrnick Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ljharb ljharb Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syrnick can you please open an issue about it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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)

@@ -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');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. notable-change PRs with changes that should be highlighted in changelogs. promises Issues and PRs related to ECMAScript promises. semver-minor PRs that contain new features and should be released in the next minor version. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support awaiting values through util.awaitable