stream: deprecate asIndexedPairs by atlowChemi · Pull Request #48102 · nodejs/node

atlowChemi · 2023-05-21T11:01:11Z

Refs: https://github.com/tc39/proposal-iterator-helpers, tc39/proposal-iterator-helpers@63567bb, tc39/proposal-iterator-helpers@bdb4883, tc39/proposal-iterator-helpers#211

nodejs-github-bot · 2023-05-21T11:01:16Z

Review requested:

@nodejs/streams

aduh95 · 2023-05-21T11:25:04Z

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

benjamingr · 2023-05-21T12:43:45Z

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

nodejs-github-bot · 2023-05-21T12:51:03Z

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

doc/api/deprecations.md

aduh95

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 · 2023-05-21T15:47:03Z

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

lib/internal/streams/operators.js

aduh95 · 2023-05-21T16:44:48Z

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 · 2023-05-21T17:16:55Z

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

aduh95 · 2023-05-21T17:21:06Z

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

I concur with @aduh95

benjamingr · 2023-05-22T16:56:15Z

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

benjamingr · 2023-05-22T16:56:51Z

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

atlowChemi · 2023-05-23T04:26:50Z

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

Yeah, makes sense 🙂

doc/api/stream.md

nodejs-github-bot · 2023-05-23T22:09:33Z

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

nodejs-github-bot · 2023-05-24T07:44:50Z

Landed in c462147

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>

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

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

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>

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>

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

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>

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

nodejs-github-bot added the needs-ci PRs that need a full CI run. label May 21, 2023

benjamingr approved these changes May 21, 2023

View reviewed changes

atlowChemi marked this pull request as ready for review May 21, 2023 11:09

atlowChemi force-pushed the runtime-deprecate-asIndexedPairs branch from a63bb8a to 217c203 Compare May 21, 2023 11:17

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

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

aduh95 reviewed May 21, 2023

View reviewed changes

doc/api/deprecations.md Outdated Show resolved Hide resolved

aduh95 reviewed May 21, 2023

View reviewed changes

atlowChemi commented May 21, 2023

View reviewed changes

lib/internal/streams/operators.js Outdated Show resolved Hide resolved

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

mcollina reviewed May 22, 2023

View reviewed changes

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

atlowChemi added 2 commits May 23, 2023 07:57

stream: deprecate asIndexedPairs

f5e17ea

CR

90c4cc6

atlowChemi force-pushed the runtime-deprecate-asIndexedPairs branch from bbbdff5 to 90c4cc6 Compare May 23, 2023 04:58

atlowChemi requested review from aduh95 and benjamingr May 23, 2023 05:00

aduh95 reviewed May 23, 2023

View reviewed changes

doc/api/stream.md Outdated Show resolved Hide resolved

aduh95 reviewed May 23, 2023

View reviewed changes

doc/api/stream.md Outdated Show resolved Hide resolved

github-actions bot mentioned this pull request May 24, 2023

CI Reliability 2023-05-24 nodejs/reliability#575

Open

19 tasks

JungMinu approved these changes May 24, 2023

View reviewed changes

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 removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 24, 2023

nodejs-github-bot merged commit c462147 into nodejs:main May 24, 2023

atlowChemi deleted the runtime-deprecate-asIndexedPairs branch May 24, 2023 08:22

atlowChemi mentioned this pull request May 24, 2023

stream: remove asIndexedPairs #48150

Merged

targos pushed a commit that referenced this pull request May 30, 2023

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 bot mentioned this pull request May 31, 2023

CI Reliability 2023-05-31 nodejs/reliability#582

Open

22 tasks

targos mentioned this pull request Jun 4, 2023

v20.3.0 release proposal #48332

Merged

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

Uh oh!

Conversation

atlowChemi commented May 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented May 21, 2023

Uh oh!

aduh95 commented May 21, 2023

Uh oh!

benjamingr commented May 21, 2023

Uh oh!

nodejs-github-bot commented May 21, 2023

Uh oh!

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

atlowChemi commented May 21, 2023

Uh oh!

Uh oh!

aduh95 commented May 21, 2023

Uh oh!

benjamingr commented May 21, 2023

Uh oh!

aduh95 commented May 21, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr commented May 22, 2023

Uh oh!

benjamingr commented May 22, 2023

Uh oh!

atlowChemi commented May 23, 2023

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented May 23, 2023

Uh oh!

nodejs-github-bot commented May 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

atlowChemi commented May 21, 2023 •

edited

Loading