Skip to content

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Jan 30, 2023

Runtime-deprecates the asyncResource property that is attached to the wrapper function returned by asyncResource.bind(). This property is not expected to align with the equivalent asyncContext.wrap() API in the proposed TC39 standard.

/cc @nodejs/async_hooks

@jasnell jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. async_hooks Issues and PRs related to the async hooks subsystem. labels Jan 30, 2023
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 30, 2023
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2023
@nodejs-github-bot

This comment was marked as outdated.

Runtime-deprecates the `asyncResource` property that is attached to
the wrapper function returned by `asyncResource.bind()`. This property
is not expected to align with the equivalent `asyncContext.wrap()`
API in the proposed TC39 standard.
@jasnell jasnell force-pushed the deprecate-asyncresource-property branch from 1e1cfc8 to 4588666 Compare February 1, 2023 14:19
@jasnell
Copy link
Member Author

jasnell commented Feb 1, 2023

/cc @nodejs/tsc

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

@jasnell jasnell added needs-ci PRs that need a full CI run. commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 1, 2023
@jasnell jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 2, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 2, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/46432
✔  Done loading data for nodejs/node/pull/46432
----------------------------------- PR info ------------------------------------
Title      async_hooks: deprecate the AsyncResource.bind asyncResource property (#46432)
Author     James M Snell  (@jasnell)
Branch     jasnell:deprecate-asyncresource-property -> nodejs:main
Labels     semver-major, async_hooks, author ready
Commits    1
 - async_hooks: deprecate the AsyncResource.bind asyncResource property
Committers 1
 - James M Snell 
PR-URL: https://github.com/nodejs/node/pull/46432
Reviewed-By: Chengzhong Wu 
Reviewed-By: Stephen Belanger 
Reviewed-By: Gerhard Stöbich 
Reviewed-By: Michaël Zasso 
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46432
Reviewed-By: Chengzhong Wu 
Reviewed-By: Stephen Belanger 
Reviewed-By: Gerhard Stöbich 
Reviewed-By: Michaël Zasso 
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 30 Jan 2023 23:25:04 GMT
   ✔  Approvals: 5
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/46432#pullrequestreview-1276302922
   ✔  - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/46432#pullrequestreview-1276606639
   ✔  - Gerhard Stöbich (@Flarna): https://github.com/nodejs/node/pull/46432#pullrequestreview-1276659985
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/46432#pullrequestreview-1279316281
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/46432#pullrequestreview-1279318704
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-01-31T07:28:43Z: https://ci.nodejs.org/job/node-test-pull-request/49273/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - async_hooks: deprecate the AsyncResource.bind asyncResource property
- Querying data for job/node-test-pull-request/49273/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/4079305303

@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 2, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 2, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 3, 2023

@jasnell jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 3, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 3, 2023
@nodejs-github-bot nodejs-github-bot merged commit 9fafb0a into nodejs:main Feb 3, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 9fafb0a

@bengl
Copy link
Member

bengl commented Nov 12, 2023

@jasnell I had opened #43065, and it was approved, but didn't land it due to some internal discussion on whether there was a better approach. While that discussion proceeded (over .. what's now many months, sorry!), it looks like this one landed. The addition of the deprecation warnings precludes the ability to use the faster ordinary object assignment in #43065.

I see two paths forward:

  1. Some other way of getting the deprectaion warning could be devised (I'm not sure this is possible in a non-breaking way).
  2. Just wait until the property is removed.

I'm leaning toward option 2, since it's trivial to construct a userland implementation of bind() that either doesn't set asyncResource or sets it more efficiently. That is, unless you've got any ideas for option 1 here?

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

Labels

async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants