Skip to content

http2: use args.This() instead of args.Holder()#58004

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
joyeecheung:fix-holder
Apr 25, 2025
Merged

http2: use args.This() instead of args.Holder()#58004
nodejs-github-bot merged 1 commit intonodejs:mainfrom
joyeecheung:fix-holder

Conversation

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Apr 24, 2025

args.Holder() has been removed in newer version of V8.

Blocks #57753

@joyeecheung
http2: use args.This() instead of args.Holder()
2062ab5 
args.Holder() has been removed in newer version of V8.
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 24, 2025

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Apr 24, 2025
@joyeecheung joyeecheung added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 24, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 24, 2025

Fast-track has been requested by @joyeecheung. Please 👍 to approve.

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 24, 2025 •
edited
Loading

This seems safe enough to fast track, as it blocks the V8 upgrade (which in turn blocks 24)

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 24, 2025
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 24, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 24, 2025

Failed to start CI
   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/14642638265

@richardlau richardlau added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Apr 24, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 24, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 24, 2025

CI: https://ci.nodejs.org/job/node-test-pull-request/66436/

@codecov
Copy link

codecov bot commented Apr 24, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.27%. Comparing base (555aeb3) to head (2062ab5).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/node_http2.cc 0.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #58004      +/-   ##
==========================================
- Coverage   90.27%   90.27%   -0.01%     
==========================================
  Files         630      630              
  Lines      186168   186168              
  Branches    36477    36475       -2     
==========================================
- Hits       168069   168065       -4     
+ Misses      10974    10964      -10     
- Partials     7125     7139      +14
Files with missing lines Coverage Δ
src/node_http2.cc 83.47% <0.00%> (+0.09%) ⬆️

... and 27 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.

@joyeecheung joyeecheung mentioned this pull request Apr 24, 2025
Closed
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 24, 2025

CI: https://ci.nodejs.org/job/node-test-pull-request/66441/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 24, 2025

CI: https://ci.nodejs.org/job/node-test-pull-request/66443/

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 24, 2025
@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 25, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 25, 2025
@nodejs-github-bot nodejs-github-bot merged commit 9cc0195 into nodejs:main Apr 25, 2025
85 checks passed
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 25, 2025

Landed in 9cc0195

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@joyeecheung @RafaelGSS
http2: use args.This() instead of args.Holder()
8c156d5 
args.Holder() has been removed in newer version of V8.

PR-URL: #58004
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@RafaelGSS RafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@joyeecheung @RafaelGSS
http2: use args.This() instead of args.Holder()
924ebcd 
args.Holder() has been removed in newer version of V8.

PR-URL: #58004
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@joyeecheung @aduh95
http2: use args.This() instead of args.Holder()
6ff5d2a 
args.Holder() has been removed in newer version of V8.

PR-URL: #58004
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@joyeecheung @aduh95
http2: use args.This() instead of args.Holder()
011f7da 
args.Holder() has been removed in newer version of V8.

PR-URL: #58004
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
@joyeecheung @RafaelGSS
http2: use args.This() instead of args.Holder()
d88e5c6 
args.Holder() has been removed in newer version of V8.

PR-URL: #58004
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 16, 2025
@joyeecheung @aduh95
http2: use args.This() instead of args.Holder()
dbb6d06 
args.Holder() has been removed in newer version of V8.

PR-URL: #58004
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@joyeecheung @aduh95
http2: use args.This() instead of args.Holder()
ec1ca7c 
args.Holder() has been removed in newer version of V8.

PR-URL: #58004
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@joyeecheung @aduh95
http2: use args.This() instead of args.Holder()
4b5a55b 
args.Holder() has been removed in newer version of V8.

PR-URL: #58004
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@joyeecheung @aduh95
http2: use args.This() instead of args.Holder()
9e9318e 
args.Holder() has been removed in newer version of V8.

PR-URL: #58004
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
aduh95 pushed a commit that referenced this pull request May 18, 2025
@joyeecheung @aduh95
http2: use args.This() instead of args.Holder()
d289119 
args.Holder() has been removed in newer version of V8.

PR-URL: #58004
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@github-actions github-actions bot mentioned this pull request May 19, 2025
Merged
aduh95 pushed a commit that referenced this pull request May 19, 2025
@joyeecheung @aduh95
http2: use args.This() instead of args.Holder()
bf9e17e 
args.Holder() has been removed in newer version of V8.

PR-URL: #58004
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@ghost ghost mentioned this pull request Jun 8, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@pimterry pimterry pimterry approved these changes

@anonrig anonrig anonrig approved these changes

@richardlau richardlau richardlau approved these changes

@legendecas legendecas legendecas approved these changes

Assignees

No one assigned

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++. fast-track PRs that do not need to wait for 48 hours to land. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@joyeecheung @nodejs-github-bot @jasnell @pimterry @anonrig @richardlau @legendecas @aduh95