Skip to content

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented May 26, 2025

These APIs have been deprecated in #18632.

Given the parameter async_context is essential for stable API AsyncLocalStorage, it'd be better to remove them now.

@legendecas legendecas added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 26, 2025
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 26, 2025
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

7+ years is likely a long enough deprecation cycle ;-)

@jasnell
Copy link
Member

jasnell commented May 26, 2025

This needs to update the corresponding entries in deprecations.md also. Deprecation code DEP0097 I believe.

@legendecas
Copy link
Member Author

This needs to update the corresponding entries in deprecations.md also. Deprecation code DEP0097 I believe.

DEP0097 is a runtime warning and is still possible with the alternative API:

node::MakeCallback(isolate, recv, method, 0, nullptr,
node::async_context{0, 0});
. So DEP0097 should still be active until we can fully deprecate domain. Or it can be deprecated in a separate PR, as it changes runtime warning -- while this PR is a fully build time deprecation.

Copy link

codecov bot commented May 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.20%. Comparing base (83052ff) to head (3dcca4c).
Report is 80 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58471      +/-   ##
==========================================
- Coverage   90.22%   90.20%   -0.02%     
==========================================
  Files         635      635              
  Lines      187313   187295      -18     
  Branches    36778    36782       +4     
==========================================
- Hits       169003   168953      -50     
- Misses      11080    11126      +46     
+ Partials     7230     7216      -14     
Files with missing lines Coverage Δ
src/api/callback.cc 82.29% <ø> (+7.05%) ⬆️
src/node.h 92.30% <ø> (ø)

... and 48 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@addaleax
Copy link
Member

I think this one you'll want to run CITGM for. This is probably still a fairly popular thing to use in extant Node.js addons.

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label May 27, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 27, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@legendecas
Copy link
Member Author

legendecas commented May 28, 2025

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3599/

No failures related to MakeCallback removal.

@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 6, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 6, 2025
@nodejs-github-bot nodejs-github-bot merged commit 708fd19 into nodejs:main Jun 6, 2025
73 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 708fd19

@legendecas legendecas deleted the node-h-async-callback branch June 6, 2025 10:59
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++. needs-ci PRs that need a full CI run. 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.

6 participants