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

src: deprecate AddPromiseHook() #26529

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Mar 8, 2019

This API was added to fill an use case that is served by async_hooks,
since that has Promise support.

Deprecate this, as the underlying Isolate::SetPromiseHook() may be
removed in its current form in the future.

Refs: https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

This API was added to fill an use case that is served by `async_hooks`,
since that has `Promise` support.

Deprecate this, as the underlying `Isolate::SetPromiseHook()` may be
removed in its current form in the future.

Refs: https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/
@addaleax addaleax added semver-minor PRs that contain new features and should be released in the next minor version. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 8, 2019
@addaleax addaleax requested a review from mcollina March 8, 2019 17:20
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 8, 2019
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

Should this go into doc/api/deprecations.md?

@addaleax
Copy link
Member Author

addaleax commented Mar 8, 2019

@mcollina I don’t think we’ve done that for C++ deprecations before, and I don’t really think we need to, given how different and more directly dev-focused C++ deprecations are from JS deprecations

@BridgeAR
Copy link
Member

BridgeAR commented Mar 8, 2019

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 8, 2019
@addaleax
Copy link
Member Author

addaleax commented Mar 9, 2019

@addaleax
Copy link
Member Author

Landed in 171b270

@addaleax addaleax closed this Mar 10, 2019
@addaleax addaleax deleted the deprecate-add-promise-hook branch March 10, 2019 21:41
addaleax added a commit that referenced this pull request Mar 10, 2019
This API was added to fill an use case that is served by `async_hooks`,
since that has `Promise` support.

Deprecate this, as the underlying `Isolate::SetPromiseHook()` may be
removed in its current form in the future.

Refs: https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/

PR-URL: #26529
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Mar 10, 2019
2 tasks
addaleax added a commit to addaleax/node that referenced this pull request Mar 21, 2019
Remove this, as the underlying `Isolate::SetPromiseHook()` may be
removed as well in its current form in the future, and `async_hooks`
also serves this use case.

Refs: https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/
Refs: nodejs#26529
addaleax added a commit that referenced this pull request Mar 21, 2019
Remove this, as the underlying `Isolate::SetPromiseHook()` may be
removed as well in its current form in the future, and `async_hooks`
also serves this use case.

Refs: https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/
Refs: #26529

PR-URL: #26574
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
This API was added to fill an use case that is served by `async_hooks`,
since that has `Promise` support.

Deprecate this, as the underlying `Isolate::SetPromiseHook()` may be
removed in its current form in the future.

Refs: https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/

PR-URL: nodejs#26529
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request Mar 28, 2019
Notable changes:

* crypto
  * Allow deriving public from private keys (Tobias Nießen)
    [#26278](#26278).
* events
  * Added a `once` function to use `EventEmitter` with promises
    (Matteo Collina) [#26078](#26078).
* tty
  * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater)
    [#26247](#26247).
  * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater)
    [#26485](#26485).
* v8
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools (James M Snell)
    [#26501](#26501).
* worker
  * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in
    different vm.Contexts, aiding with the isolation that the vm module seeks to
    provide (Anna Henningsen)
    [#26497](#26497).
* C++ API
  * `AddPromiseHook` is now deprecated. This API was added to fill an use case
    that is served by `async_hooks`, since that has `Promise` support
    (Anna Henningsen) [#26529](#26529).
  * Added a `Stop` API to shut down Node.js while it is running
    (Gireesh Punathil) [#21283](#21283).
* meta
  * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of
    the Technical Steering Committee
    [#26657](#26657).
  * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators
    [#26730](#26730).

PR-URL: #26949
targos added a commit that referenced this pull request Mar 28, 2019
Notable changes:

* crypto
  * Allow deriving public from private keys (Tobias Nießen)
    [#26278](#26278).
* events
  * Added a `once` function to use `EventEmitter` with promises
    (Matteo Collina) [#26078](#26078).
* tty
  * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater)
    [#26247](#26247).
  * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater)
    [#26485](#26485).
* v8
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools (James M Snell)
    [#26501](#26501).
* worker
  * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in
    different vm.Contexts, aiding with the isolation that the vm module seeks to
    provide (Anna Henningsen)
    [#26497](#26497).
* C++ API
  * `AddPromiseHook` is now deprecated. This API was added to fill an use case
    that is served by `async_hooks`, since that has `Promise` support
    (Anna Henningsen) [#26529](#26529).
  * Added a `Stop` API to shut down Node.js while it is running
    (Gireesh Punathil) [#21283](#21283).
* meta
  * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of
    the Technical Steering Committee
    [#26657](#26657).
  * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators
    [#26730](#26730).

PR-URL: #26949
BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
Notable changes:

* crypto
  * Allow deriving public from private keys (Tobias Nießen)
    [#26278](#26278).
* events
  * Added a `once` function to use `EventEmitter` with promises
    (Matteo Collina) [#26078](#26078).
* tty
  * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater)
    [#26247](#26247).
  * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater)
    [#26485](#26485).
* v8
  * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots
    in the format used by tools such as Chrome DevTools (James M Snell)
    [#26501](#26501).
* worker
  * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in
    different vm.Contexts, aiding with the isolation that the vm module seeks to
    provide (Anna Henningsen)
    [#26497](#26497).
* C++ API
  * `AddPromiseHook` is now deprecated. This API was added to fill an use case
    that is served by `async_hooks`, since that has `Promise` support
    (Anna Henningsen) [#26529](#26529).
  * Added a `Stop` API to shut down Node.js while it is running
    (Gireesh Punathil) [#21283](#21283).
* meta
  * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of
    the Technical Steering Committee
    [#26657](#26657).
  * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators
    [#26730](#26730).

PR-URL: #26949
BethGriggs pushed a commit that referenced this pull request Apr 16, 2019
This API was added to fill an use case that is served by `async_hooks`,
since that has `Promise` support.

Deprecate this, as the underlying `Isolate::SetPromiseHook()` may be
removed in its current form in the future.

Refs: https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/

PR-URL: #26529
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants