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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

buffer: move SlowBuffer to EOL #58008

wants to merge 2 commits into from
+61 −118

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
doc/api/deprecations.md
@@ -706,9 +709,9 @@ changes:
description: Documentation-only deprecation.
-->

Type: Runtime
Type: End-of-Life
Copy link
Member

Choose a reason for hiding this comment

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

The PR that makes this EOL comment is missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not following. The pr-url is specified in the changes block.

@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 4 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
Copy link
Collaborator

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

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

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

@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. 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.
9 participants
@jasnell @nodejs-github-bot @marco-ippolito @mcollina @anonrig @cjihrig @ronag @RaisinTen @bjohansebas