Skip to content

stream: remove unused _transformState - #33105

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:stream-transform
Closed

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

Conversation

@ronag

@ronagronag 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

@ronag
stream: remove unused _transformState
5b92a54
_transformState is no longer used since Transform was
simplified.
Refs: nodejs#32763
@ronagronag added the stream Issues and PRs related to the stream subsystem. label Apr 27, 2020
@ronag

ronag commented Apr 28, 2020

Copy link
Copy Markdown
MemberAuthor

@nodejs/streams

@lpinca

lpinca commented Apr 28, 2020

Copy link
Copy Markdown
Member

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

mcollina
mcollina reviewed Apr 28, 2020
View reviewed changes

@mcollinamcollina 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.

lgtm

Does this depend on a semver-major commit?

@ronag

ronag commented Apr 28, 2020

Copy link
Copy Markdown
MemberAuthor

@mcollina: yes, #32763 is semver-major

@ronag

ronag commented Apr 28, 2020

Copy link
Copy Markdown
MemberAuthor

Did #32763 land on v14?

@jasnell

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

ronag commented Apr 28, 2020

Copy link
Copy Markdown
MemberAuthor

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

@jasnell

jasnell commented Apr 28, 2020•
edited
Loading

Copy link
Copy Markdown
Member

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

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

ronag commented Apr 30, 2020

Copy link
Copy Markdown
MemberAuthor

@mcollina: Was that lgtm an approve?

mcollina
mcollina approved these changes Apr 30, 2020
View reviewed changes

@mcollinamcollina 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.

lgtm, yes

@ronagronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 30, 2020
@nodejs-github-bot

nodejs-github-bot commented Apr 30, 2020

Copy link
Copy Markdown
Collaborator

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

@ronag

ronag commented Apr 30, 2020

Copy link
Copy Markdown
MemberAuthor

Landed in e10e292

@ronagronag closed this Apr 30, 2020
ronag added a commit that referenced this pull request Apr 30, 2020
@ronag
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
@ronag
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
@ronag@codebytere
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
@ronag@codebytere
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
@bjohansebasbjohansebas mentioned this pull request May 31, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@lpincalpincalpinca approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.streamIssues 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

@ronag@lpinca@jasnell@nodejs-github-bot@mcollina