FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: remove unused _transformState by ronag · Pull Request #33105 · nodejs/node · GitHub

/ node Public

stream: remove unused _transformState - #33105

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:stream-transform
ConversationCommits (1)ChecksFiles changed
Closed

stream: remove unused _transformState#33105
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:stream-transform

Conversation

ronag commented Apr 27, 2020

Copy link
Copy Markdown
Member

_transformState is no longer used since Transform was simplified.

Refs: #32763

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

stream: remove unused _transformState
5b92a54 
_transformState is no longer used since Transform was
simplified.

Refs: nodejs#32763
ronag added the stream Issues and PRs related to the stream subsystem. label Apr 27, 2020

ronag commented Apr 28, 2020

Copy link
Copy Markdown
Member Author

@nodejs/streams

lpinca commented Apr 28, 2020

Copy link
Copy Markdown
Member

I wonder if _transformState is used in the wild, otherwise LGTM.

mcollina reviewed Apr 28, 2020
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

Does this depend on a semver-major commit?

ronag commented Apr 28, 2020

Copy link
Copy Markdown
Member Author

@mcollina: yes, #32763 is semver-major

ronag commented Apr 28, 2020

Copy link
Copy Markdown
Member Author

Did #32763 land on v14?

jasnell commented Apr 28, 2020

Copy link
Copy Markdown
Member

Oy, I wish I had caught #32763 before it landed. Even tho _transformState was a private property, the fact that it was public for so long really means that it should ideally have gone through a deprecation cycle as opposed to being removed immediately. I just confirmed that _transformState is still present in Node.js 14.0.0. I absolutely think we should follow a proper runtime deprecation cycle for _transformState in 14.x.

ronag commented Apr 28, 2020

Copy link
Copy Markdown
Member Author

@jasnell: I'll open a separate PR for runtime deprecation against v14. Can it still land there though?

jasnell commented Apr 28, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

The @nodejs/tsc will have to decide. We've done it before.

ronag mentioned this pull request Apr 28, 2020
Closed
4 tasks
ronag requested a review from mcollina April 29, 2020 09:18

ronag commented Apr 30, 2020

Copy link
Copy Markdown
Member Author

@mcollina: Was that lgtm an approve?

mcollina approved these changes Apr 30, 2020
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm, yes

ronag added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 30, 2020

nodejs-github-bot commented Apr 30, 2020

Copy link
Copy Markdown
Collaborator

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

ronag commented Apr 30, 2020

Copy link
Copy Markdown
Member Author

Landed in e10e292

ronag closed this Apr 30, 2020
ronag added a commit that referenced this pull request Apr 30, 2020
stream: remove unused _transformState
e10e292 
_transformState is no longer used since Transform was
simplified.

Refs: #32763

PR-URL: #33105
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
ronag added a commit to nxtedition/node that referenced this pull request Jun 28, 2020
stream: runtime deprecate Transform._transformState
b3a32c2 
Transform._transformState is removed in future version as part
of a refactoring.

Refs: nodejs#32763
Refs: nodejs#33105 (comment)
codebytere pushed a commit that referenced this pull request Jun 30, 2020
stream: runtime deprecate Transform._transformState
db2d1ca 
Transform._transformState is removed in future version as part
of a refactoring.

Refs: #32763
Refs: #33105 (comment)

Backport-PR-URL: #33126
PR-URL: #32763
codebytere pushed a commit that referenced this pull request Jun 30, 2020
stream: runtime deprecate Transform._transformState
9637ee6 
Transform._transformState is removed in future version as part
of a refactoring.

Refs: #32763
Refs: #33105 (comment)

Backport-PR-URL: #33126
PR-URL: #32763
bjohansebas mentioned this pull request May 31, 2025
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
lpinca lpinca approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL