Skip to content

lib: flag to conditionally modify proto on deprecate #58928

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

Conversation

RafaelGSS
Copy link
Member

Refs: #58218

In this specific benchmark, the performance difference between modifyPrototype=0 and modifyPrototype=1 is marginally significant at the 5% level:

  • Mean (modifyPrototype=0): 390,196.99 ops/sec
  • Mean (modifyPrototype=1): 382,521.21 ops/sec
  • p-value: 0.05087

While this case is borderline, other benchmarks (e.g., #58907) have shown a more noticeable improvement, so I think this worth the change.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Jul 2, 2025
Copy link

codecov bot commented Jul 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.07%. Comparing base (a7a37c3) to head (b782bc8).
Report is 42 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58928      +/-   ##
==========================================
- Coverage   90.11%   90.07%   -0.04%     
==========================================
  Files         640      640              
  Lines      188427   188466      +39     
  Branches    36956    36969      +13     
==========================================
- Hits       169792   169765      -27     
- Misses      11348    11426      +78     
+ Partials     7287     7275      -12     
Files with missing lines Coverage Δ
lib/internal/util.js 96.16% <100.00%> (-0.68%) ⬇️

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS RafaelGSS added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. review wanted PRs that need reviews. and removed help wanted Issues that need assistance from volunteers or PRs that need help to proceed. labels Jul 5, 2025
@RafaelGSS RafaelGSS added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 7, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 8, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/58928
✔  Done loading data for nodejs/node/pull/58928
----------------------------------- PR info ------------------------------------
Title      lib: flag to conditionally modify proto on deprecate (#58928)
Author     Rafael Gonzaga <[email protected]> (@RafaelGSS)
Branch     RafaelGSS:conditionally-modify-prototype-deprecate -> nodejs:main
Labels     util, author ready, needs-ci, review wanted
Commits    1
 - lib: flag to conditionally modify proto on deprecate
Committers 1
 - RafaelGSS <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/58928
Refs: https://github.com/nodejs/node/issues/58218
Reviewed-By: Juan José Arboleda <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/58928
Refs: https://github.com/nodejs/node/issues/58218
Reviewed-By: Juan José Arboleda <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 02 Jul 2025 14:55:12 GMT
   ✔  Approvals: 1
   ✔  - Juan José Arboleda (@juanarbol): https://github.com/nodejs/node/pull/58928#pullrequestreview-2995063544
   ✘  This PR needs to wait 25 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-07-03T19:58:05Z: https://ci.nodejs.org/job/node-test-pull-request/67827/
- Querying data for job/node-test-pull-request/67827/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/16145176087

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jul 8, 2025
@RafaelGSS RafaelGSS removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jul 8, 2025
@RafaelGSS RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 8, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 8, 2025
@nodejs-github-bot nodejs-github-bot merged commit eff504f into nodejs:main Jul 8, 2025
85 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in eff504f

targos pushed a commit that referenced this pull request Jul 17, 2025
Refs: #58218
Signed-off-by: RafaelGSS <[email protected]>
PR-URL: #58928
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
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. needs-ci PRs that need a full CI run. review wanted PRs that need reviews. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants