Skip to content

lib: runtime deprecate SlowBuffer #55175

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
Merged
merged 3 commits into from
Oct 15, 2024
Merged

lib: runtime deprecate SlowBuffer #55175

merged 3 commits into from
Oct 15, 2024

Conversation

RafaelGSS
Copy link
Member

SlowBuffer has been doc-deprecated for a while and I couldn't find why we didn't want to proceed with the deprecation cycle on this feature. Therefore, this PR runtime deprecates SlowBuffer.

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. labels Sep 30, 2024
@RafaelGSS RafaelGSS force-pushed the runtime-deprecate-slow-buffer branch from fd4ffe7 to b409b7e Compare September 30, 2024 13:09
@RafaelGSS RafaelGSS added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 30, 2024
@RafaelGSS
Copy link
Member Author

Tagging @nodejs/tsc as it's a semver-major that might land on Node.js 23.

avivkeller
avivkeller reviewed Sep 30, 2024
View reviewed changes
@RafaelGSS RafaelGSS mentioned this pull request Sep 30, 2024
Closed
@avivkeller avivkeller added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 30, 2024
@aduh95 aduh95 added the needs-citgm PRs that need a CITGM CI run. label Sep 30, 2024
@avivkeller avivkeller added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2024
@nodejs-github-bot
Copy link
Collaborator

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

avivkeller
avivkeller reviewed Sep 30, 2024
View reviewed changes
BridgeAR
BridgeAR requested changes Sep 30, 2024
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

The deprecation sounds good to me. Let's run CITGM anyway and please add a test as well.

@RafaelGSS
Copy link
Member Author

RafaelGSS commented Sep 30, 2024 •
edited
Loading

CITGM: https://ci.nodejs.org/job/citgm-smoker/3478/
CITGM(v22.x): https://ci.nodejs.org/job/citgm-smoker/3479/ -- for comparisson

mcollina
mcollina approved these changes Oct 1, 2024
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

@RafaelGSS
Copy link
Member Author

CITGM results:

FAILURE: 79 failures in 3478 not present in 3479


┌────────────────────────┬─────────────────────┬─────────────────────┬────────────────────────────────┬───────────────────────┬─────────────────────┬───────────────────┬──────────────────────────┬──────────────────┬───────────────┬───────────────────┬───────────────┬───────────────┬───────────────┬───────────────────────┬──────────────────┬───────────────┬──────────────┐
│ (index)                │ 0                   │ 1                   │ 2                              │ 3                     │ 4                   │ 5                 │ 6                        │ 7                │ 8             │ 9                 │ 10            │ 11            │ 12            │ 13                    │ 14               │ 15            │ 16           │
├────────────────────────┼─────────────────────┼─────────────────────┼────────────────────────────────┼───────────────────────┼─────────────────────┼───────────────────┼──────────────────────────┼──────────────────┼───────────────┼───────────────────┼───────────────┼───────────────┼───────────────┼───────────────────────┼──────────────────┼───────────────┼──────────────┤
│ fedora-last-latest-x64 │ '@yarnpkg/cli'      │ 'gulp-v5.0.0'       │ 'jest-v29.7.0'                 │ 'lru-cache-v11.0.1'   │ 'mime-v4.0.4'       │ 'nan-v2.20.0'     │ 'pino-v9.4.0'            │ 'weak-v1.0.1'    │ 'ws-v8.18.0'  │                   │               │               │               │                       │                  │               │              │
│ debian12-x64           │ '@yarnpkg/cli'      │ 'fastify-v5.0.0'    │ 'import-in-the-middle-v1.11.2' │ 'jest-v29.7.0'        │ 'lru-cache-v11.0.1' │ 'mime-v4.0.4'     │ 'nan-v2.20.0'            │ 'undici-v6.19.8' │ 'weak-v1.0.1' │ 'ws-v8.18.0'      │               │               │               │                       │                  │               │              │
│ osx11                  │ '@yarnpkg/cli'      │ 'lru-cache-v11.0.1' │ 'mime-v4.0.4'                  │ 'nan-v2.20.0'         │ 'undici-v6.19.8'    │ 'weak-v1.0.1'     │ 'ws-v8.18.0'             │                  │               │                   │               │               │               │                       │                  │               │              │
│ win-vs2022             │ 'resolve-v1.22.8'   │ 'weak-v1.0.1'       │                                │                       │                     │                   │                          │                  │               │                   │               │               │               │                       │                  │               │              │
│ fedora-latest-x64      │ '@hapi/shot-v6.0.1' │ '@yarnpkg/cli'      │ 'bcrypt-v5.1.1'                │ 'binary-split-v1.0.5' │ 'bl-v6.0.16'        │ 'inherits-v2.0.4' │ 'is-core-module-v2.15.1' │ 'jest-v29.7.0'   │ 'koa-v2.15.3' │ 'lodash-v4.17.21' │ 'mime-v4.0.4' │ 'nan-v2.20.0' │ 'pino-v9.4.0' │ 'prom-client-v15.1.3' │ 'pumpify-v2.0.1' │ 'weak-v1.0.1' │ 'ws-v8.18.0' │
│ rhel8-ppc64le          │ '@yarnpkg/cli'      │ 'lru-cache-v11.0.1' │ 'mime-v4.0.4'                  │ 'nan-v2.20.0'         │ 'weak-v1.0.1'       │ 'ws-v8.18.0'      │                          │                  │               │                   │               │               │               │                       │                  │               │              │
│ rhel8-x64              │ '@yarnpkg/cli'      │ 'jest-v29.7.0'      │ 'lru-cache-v11.0.1'            │ 'mime-v4.0.4'         │ 'nan-v2.20.0'       │ 'pino-v9.4.0'     │ 'weak-v1.0.1'            │ 'ws-v8.18.0'     │               │                   │               │               │               │                       │                  │               │              │
│ ubuntu2204-64          │ '@yarnpkg/cli'      │ 'jest-v29.7.0'      │ 'lru-cache-v11.0.1'            │ 'mime-v4.0.4'         │ 'nan-v2.20.0'       │ 'weak-v1.0.1'     │ 'ws-v8.18.0'             │                  │               │                   │               │               │               │                       │                  │               │              │
│ rhel8-s390x            │ '@yarnpkg/cli'      │ 'lru-cache-v11.0.1' │ 'mime-v4.0.4'                  │ 'weak-v1.0.1'         │ 'ws-v8.18.0'        │                   │                          │                  │               │                   │               │               │               │                       │                  │               │              │
│ alpine-latest-x64      │ 'jest-v29.7.0'      │ 'lru-cache-v11.0.1' │ 'mime-v4.0.4'                  │ 'nan-v2.20.0'         │ 'weak-v1.0.1'       │ 'ws-v8.18.0'      │                          │                  │               │                   │               │               │               │                       │                  │               │              │
│ aix72-ppc64            │ 'undici-v6.19.8'    │ 'weak-v1.0.1'       │                                │                       │                     │                   │                          │                  │               │                   │               │               │               │                       │                  │               │              │
└────────────────────────┴─────────────────────┴─────────────────────┴────────────────────────────────┴───────────────────────┴─────────────────────┴───────────────────┴──────────────────────────┴──────────────────┴───────────────┴───────────────────┴───────────────┴───────────────┴───────────────┴───────────────────────┴──────────────────┴───────────────┴──────────────┘
These modules failed in all platforms: weak-v1.0.1

But, looking at those errors, it doesn't seem related to this change, but to the main branch. I'll spin another CITGM for main and compare.

@RafaelGSS
Copy link
Member Author

https://ci.nodejs.org/job/citgm-smoker/3483/

@RafaelGSS
Copy link
Member Author 

FAILURE: 7 failures in 3478 not present in 3483


┌────────────────────────┬────────────────────────────────┬─────────────────┬───────────────────────┬──────────────┬──────────────────┬───────────────────┐
│ (index)                │ 0                              │ 1               │ 2                     │ 3            │ 4                │ 5                 │
├────────────────────────┼────────────────────────────────┼─────────────────┼───────────────────────┼──────────────┼──────────────────┼───────────────────┤
│ fedora-last-latest-x64 │                                │                 │                       │              │                  │                   │
│ debian12-x64           │ 'import-in-the-middle-v1.11.2' │                 │                       │              │                  │                   │
│ osx11                  │                                │                 │                       │              │                  │                   │
│ win-vs2022             │                                │                 │                       │              │                  │                   │
│ fedora-latest-x64      │ '@hapi/shot-v6.0.1'            │ 'bcrypt-v5.1.1' │ 'binary-split-v1.0.5' │ 'bl-v6.0.16' │ 'fastify-v5.0.0' │ 'lodash-v4.17.21' │
│ rhel8-ppc64le          │                                │                 │                       │              │                  │                   │
│ rhel8-x64              │                                │                 │                       │              │                  │                   │
│ ubuntu2204-64          │                                │                 │                       │              │                  │                   │
│ rhel8-s390x            │                                │                 │                       │              │                  │                   │
│ alpine-latest-x64      │                                │                 │                       │              │                  │                   │
│ aix72-ppc64            │                                │                 │                       │              │                  │                   │
└────────────────────────┴────────────────────────────────┴─────────────────┴───────────────────────┴──────────────┴──────────────────┴───────────────────┘

It seems ok to land on main. @BridgeAR

@BridgeAR
Copy link
Member

BridgeAR commented Oct 4, 2024

@RafaelGSS could you please add a test for the deprecation? We have a couple of tests like that. Otherwise it's fine to land

@RafaelGSS
Copy link
Member Author

@RafaelGSS could you please add a test for the deprecation? We have a couple of tests like that. Otherwise it's fine to land

1817d2d.

@BridgeAR PTAL. I will do the last sync for v23 today and I'd like to have it included

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@RafaelGSS RafaelGSS force-pushed the runtime-deprecate-slow-buffer branch from 1817d2d to 446244a Compare October 10, 2024 13:48
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@RafaelGSS RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 15, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 15, 2024
@nodejs-github-bot nodejs-github-bot merged commit 019efe1 into nodejs:main Oct 15, 2024
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 019efe1

@RafaelGSS RafaelGSS mentioned this pull request Mar 24, 2025
Merged
@panva panva mentioned this pull request May 7, 2025
Merged
@miyu4u miyu4u mentioned this pull request May 7, 2025
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ronag ronag ronag approved these changes

@mcollina mcollina mcollina approved these changes

@anonrig anonrig anonrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+1 more reviewer

@avivkeller avivkeller avivkeller left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem. 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.

9 participants

@RafaelGSS @nodejs-github-bot @BridgeAR @richardlau @mcollina @anonrig @ronag @avivkeller @aduh95