Skip to content
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

stream: deprecate asIndexedPairs #48102

Merged
merged 3 commits into from May 24, 2023
Merged

stream: deprecate asIndexedPairs #48102

merged 3 commits into from May 24, 2023

Conversation

atlowChemi
Copy link
Member

@atlowChemi atlowChemi commented May 21, 2023 •
edited

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label May 21, 2023
@atlowChemi atlowChemi marked this pull request as ready for review May 21, 2023 11:09
@atlowChemi atlowChemi force-pushed the runtime-deprecate-asIndexedPairs branch from a63bb8a to 217c203 Compare May 21, 2023 11:17
@aduh95
Copy link
Contributor

aduh95 commented May 21, 2023

It think this should ref tc39/proposal-iterator-helpers@63567bb.

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label May 21, 2023
@benjamingr
Copy link
Member

@aduh95 there was a future change removing it entirely rather than renaming it

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 21, 2023
@nodejs-github-bot
Copy link
Collaborator

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

aduh95
aduh95 reviewed May 21, 2023
View reviewed changes
doc/api/deprecations.md Outdated Show resolved Hide resolved
aduh95
aduh95 reviewed May 21, 2023
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

I don’t think we have a precedent for deprecating an experimental feature – and I don’t think we should set one. What I suggest we do instead is to add a runtime warning (and possibly backport it on all non-EOL release lines), then remove it entirely in a follow up PR with the dont-land-on-20.x label.

@atlowChemi
Copy link
Member Author

I don’t think we have a precedent for deprecating an experimental feature – and I don’t think we should set one. What I suggest we do instead is to add a runtime warning (and possibly backport it on all non-EOL release lines), then remove it entirely in a follow up PR with the dont-land-on-20.x label.

I think #47740 which added DEP0173 had deprecated an experimental feature https://github.com/nodejs/node/pull/47740/files#diff-a1dd65347b3a99f8969799e35dc0c324dfaa28865a0706671183aca7aeebea97R231

@atlowChemi atlowChemi force-pushed the runtime-deprecate-asIndexedPairs branch from 217c203 to bbbdff5 Compare May 21, 2023 16:35
@aduh95
Copy link
Contributor

aduh95 commented May 21, 2023

I think #47740 which added DEP0173 had deprecated an experimental feature

I still think it's a wrong move (it's too bad I missed #47740), using the deprecation cycle for an experimental feature gives the wrong message, there should be no guarantee for experimental features. I won't block this if others feel differently, count ma as -0.5.

@benjamingr
Copy link
Member

@aduh95 how would you like this removal to look, ideally?

@aduh95
Copy link
Contributor

aduh95 commented May 21, 2023

What I've written in #48102 (review):

What I suggest we do instead is to add a runtime warning (and possibly backport it on all non-EOL release lines), then remove it entirely in a follow up PR with the dont-land-on-20.x label.

mcollina
mcollina reviewed May 22, 2023
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.

I concur with @aduh95

@benjamingr
Copy link
Member

@atlowChemi @aduh95 's suggestion seems reasonable to me, WDYT?

@benjamingr benjamingr added the blocked PRs that are blocked by other issues or PRs. label May 22, 2023
@benjamingr
Copy link
Member

Added "blocked" since this has a green review (by me) but we're still discussing how this removal should happen

@atlowChemi
Copy link
Member Author

@atlowChemi @aduh95 's suggestion seems reasonable to me, WDYT?

Yeah, makes sense 🙂

@atlowChemi atlowChemi force-pushed the runtime-deprecate-asIndexedPairs branch from bbbdff5 to 90c4cc6 Compare May 23, 2023 04:58
aduh95
aduh95 reviewed May 23, 2023
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
aduh95
aduh95 reviewed May 23, 2023
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request May 24, 2023
Open
19 tasks
@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels May 24, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 24, 2023
@nodejs-github-bot nodejs-github-bot merged commit c462147 into nodejs:main May 24, 2023
64 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in c462147

@atlowChemi atlowChemi deleted the runtime-deprecate-asIndexedPairs branch May 24, 2023 08:22
@atlowChemi atlowChemi mentioned this pull request May 24, 2023
Merged
This was referenced May 25, 2023
Open
Open
Open
Open
Open
Open
targos pushed a commit that referenced this pull request May 30, 2023
@atlowChemi @targos
stream: deprecate asIndexedPairs
9e2b13d 
PR-URL: #48102
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@github-actions github-actions bot mentioned this pull request May 31, 2023
Open
22 tasks
targos added a commit that referenced this pull request Jun 4, 2023
@targos
2023-06-05, Version 20.3.0 (Current)
ca6006b 
Notable changes:

deps:
  * upgrade to libuv 1.45.0 (Santiago Gimeno) #48078
lib:
  * (SEMVER-MINOR) implement AbortSignal.any() (Chemi Atlow) #47821
module:
  * change default resolver to not throw on unknown scheme (Gil Tayar) #47824
node-api:
  * (SEMVER-MINOR) define version 9 (Chengzhong Wu) #48151
stream:
  * deprecate asIndexedPairs (Chemi Atlow) #48102

PR-URL: #48332
@targos targos mentioned this pull request Jun 4, 2023
Merged
targos added a commit that referenced this pull request Jun 5, 2023
@targos
2023-06-05, Version 20.3.0 (Current)
6d770b5 
Notable changes:

deps:
  * upgrade to libuv 1.45.0, including significant performance
    improvements to file system operations on Linux (Santiago Gimeno) #48078
doc:
  * add Ruy Adorno to list of TSC members (Michael Dawson) #48172
  * mark Node.js 14 as End-of-Life (Richard Lau) #48023
lib:
  * (SEMVER-MINOR) implement AbortSignal.any() (Chemi Atlow) #47821
module:
  * change default resolver to not throw on unknown scheme (Gil Tayar) #47824
node-api:
  * (SEMVER-MINOR) define version 9 (Chengzhong Wu) #48151
stream:
  * deprecate asIndexedPairs (Chemi Atlow) #48102

PR-URL: #48332
RafaelGSS pushed a commit that referenced this pull request Jun 7, 2023
@targos @RafaelGSS
2023-06-08, Version 20.3.0 (Current)
57679e1 
Notable changes:

deps:
  * upgrade to libuv 1.45.0, including significant performance
    improvements to file system operations on Linux (Santiago Gimeno) #48078
doc:
  * add Ruy Adorno to list of TSC members (Michael Dawson) #48172
  * mark Node.js 14 as End-of-Life (Richard Lau) #48023
lib:
  * (SEMVER-MINOR) implement AbortSignal.any() (Chemi Atlow) #47821
module:
  * change default resolver to not throw on unknown scheme (Gil Tayar) #47824
node-api:
  * (SEMVER-MINOR) define version 9 (Chengzhong Wu) #48151
stream:
  * deprecate asIndexedPairs (Chemi Atlow) #48102

PR-URL: #48332
RafaelGSS pushed a commit that referenced this pull request Jun 8, 2023
@targos @RafaelGSS
2023-06-08, Version 20.3.0 (Current)
5a58972 
Notable changes:

deps:
  * upgrade to libuv 1.45.0, including significant performance
    improvements to file system operations on Linux (Santiago Gimeno) #48078
doc:
  * add Ruy Adorno to list of TSC members (Michael Dawson) #48172
  * mark Node.js 14 as End-of-Life (Richard Lau) #48023
lib:
  * (SEMVER-MINOR) implement AbortSignal.any() (Chemi Atlow) #47821
module:
  * change default resolver to not throw on unknown scheme (Gil Tayar) #47824
node-api:
  * (SEMVER-MINOR) define version 9 (Chengzhong Wu) #48151
stream:
  * deprecate asIndexedPairs (Chemi Atlow) #48102

PR-URL: #48332
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@atlowChemi @danielleadams
stream: deprecate asIndexedPairs
6509d4e 
PR-URL: #48102
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@atlowChemi @MoLow
stream: deprecate asIndexedPairs
585d628 
PR-URL: nodejs#48102
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@atlowChemi @Ceres6
stream: deprecate asIndexedPairs
6a29340 
PR-URL: nodejs#48102
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@targos @Ceres6
2023-06-08, Version 20.3.0 (Current)
ad26015 
Notable changes:

deps:
  * upgrade to libuv 1.45.0, including significant performance
    improvements to file system operations on Linux (Santiago Gimeno) nodejs#48078
doc:
  * add Ruy Adorno to list of TSC members (Michael Dawson) nodejs#48172
  * mark Node.js 14 as End-of-Life (Richard Lau) nodejs#48023
lib:
  * (SEMVER-MINOR) implement AbortSignal.any() (Chemi Atlow) nodejs#47821
module:
  * change default resolver to not throw on unknown scheme (Gil Tayar) nodejs#47824
node-api:
  * (SEMVER-MINOR) define version 9 (Chengzhong Wu) nodejs#48151
stream:
  * deprecate asIndexedPairs (Chemi Atlow) nodejs#48102

PR-URL: nodejs#48332
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@atlowChemi @Ceres6
stream: deprecate asIndexedPairs
60c58a4 
PR-URL: nodejs#48102
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@targos @Ceres6
2023-06-08, Version 20.3.0 (Current)
902f572 
Notable changes:

deps:
  * upgrade to libuv 1.45.0, including significant performance
    improvements to file system operations on Linux (Santiago Gimeno) nodejs#48078
doc:
  * add Ruy Adorno to list of TSC members (Michael Dawson) nodejs#48172
  * mark Node.js 14 as End-of-Life (Richard Lau) nodejs#48023
lib:
  * (SEMVER-MINOR) implement AbortSignal.any() (Chemi Atlow) nodejs#47821
module:
  * change default resolver to not throw on unknown scheme (Gil Tayar) nodejs#47824
node-api:
  * (SEMVER-MINOR) define version 9 (Chengzhong Wu) nodejs#48151
stream:
  * deprecate asIndexedPairs (Chemi Atlow) nodejs#48102

PR-URL: nodejs#48332
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina left review comments

@JungMinu JungMinu JungMinu approved these changes

@aduh95 aduh95 aduh95 approved these changes

@benjamingr benjamingr Awaiting requested review from benjamingr

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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.

None yet
6 participants
@atlowChemi @nodejs-github-bot @aduh95 @benjamingr @mcollina @JungMinu