Skip to content

buffer: move SlowBuffer to EOL #58008

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

buffer: move SlowBuffer to EOL #58008

wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 24, 2025

SlowBuffer has been deprecated for many years now. Let's remove it.

@jasnell
buffer: move SlowBuffer to EOL
6be6ddc 
`SlowBuffer` has been deprecated for many years now. Let's remove it.
@jasnell jasnell added buffer Issues and PRs related to the buffer subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. deprecations Issues and PRs related to deprecations. labels Apr 24, 2025
@jasnell jasnell requested review from mcollina, anonrig and cjihrig April 24, 2025 14:52
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/config
  • @nodejs/performance
  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. config Issues or PRs related to the config subsystem needs-ci PRs that need a full CI run. labels Apr 24, 2025
jasnell
jasnell commented Apr 24, 2025
View reviewed changes
@nodejs-github-bot
Copy link
Collaborator

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

anonrig
anonrig approved these changes Apr 24, 2025
View reviewed changes
@marco-ippolito
Copy link
Member

With a quick github search it seems its safe to remove
https://github.com/search?q=SlowBuffer+language%3AJavaScript&type=code&l=JavaScript

@codecov Codecov
Copy link

codecov bot commented Apr 24, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.27%. Comparing base (b665127) to head (921e0a0).
Report is 16 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #58008   +/-   ##
=======================================
  Coverage   90.27%   90.27%           
=======================================
  Files         630      630           
  Lines      186158   186142   -16     
  Branches    36475    36474    -1     
=======================================
+ Hits       168047   168048    +1     
- Misses      10980    10982    +2     
+ Partials     7131     7112   -19
Files with missing lines Coverage Δ
lib/buffer.js 100.00% <100.00%> (ø)
src/node_options.cc 85.23% <ø> (-0.13%) ⬇️

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

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 24, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
RaisinTen
RaisinTen approved these changes Apr 25, 2025
View reviewed changes
Copy link
Member

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

Looks good, just 1 comment about the tests

test/pummel/test-buffer-large-size-slowbuffer.js
Copy link
Member

@RaisinTen RaisinTen Apr 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This test can be deleted because now it is the same as test/pummel/test-buffer-large-size-buffer-alloc-unsafe-slow.js. Both of these were created in #57789 when a test containing same calls to SlowBuffer as well as allocUnsafeSlow was split into separate files for reliability.

mcollina
mcollina approved these changes Apr 25, 2025
View reviewed changes
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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 26, 2025 •
edited by jasnell
Loading

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

@bjohansebas bjohansebas added the needs-citgm PRs that need a CITGM CI run. label Apr 26, 2025
@jasnell
Copy link
Member Author

jasnell commented Apr 27, 2025

Running citgm on this one just to be thorough but given the github codesearch results for "SlowBuffer" show no significant hits anywhere, I think it's safe to go ahead and merge. We can revert if citgm ends up showing anything but it would be an extreme surprise if it did.

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

jasnell added a commit that referenced this pull request Apr 27, 2025
@jasnell
buffer: move SlowBuffer to EOL
647175e 
`SlowBuffer` has been deprecated for many years now. Let's remove it.

PR-URL: #58008
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Apr 27, 2025

Landed in 647175e

@jasnell jasnell closed this Apr 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@lpinca lpinca lpinca approved these changes

@anonrig anonrig anonrig approved these changes

@cjihrig cjihrig cjihrig approved these changes

@ronag ronag ronag approved these changes

@marco-ippolito marco-ippolito marco-ippolito approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

@bjohansebas bjohansebas bjohansebas 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. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. config Issues or PRs related to the config subsystem deprecations Issues and PRs related to deprecations. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@jasnell @nodejs-github-bot @marco-ippolito @mcollina @lpinca @anonrig @cjihrig @ronag @RaisinTen @bjohansebas