Skip to content

[v14.x] stream: runtime deprecate Transform._transformState#33126

Closed
ronag wants to merge 1 commit into
nodejs:v14.x-stagingfrom
nxtedition:v14.x-runtime-deprecate-transformState
Closed

[v14.x] stream: runtime deprecate Transform._transformState#33126
ronag wants to merge 1 commit into
nodejs:v14.x-stagingfrom
nxtedition:v14.x-runtime-deprecate-transformState

Conversation

@ronag

@ronag ronag commented Apr 28, 2020

Copy link
Copy Markdown
Member

stream: runtime deprecate Transform._transformState

Transform._transformState is removed in future version as part of a refactoring.

Refs: #32763
Refs: #33105 (comment)

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 ronag added the stream Issues and PRs related to the stream subsystem. label Apr 28, 2020
@ronag ronag requested a review from jasnell April 28, 2020 15:41
@ronag ronag force-pushed the v14.x-runtime-deprecate-transformState branch 3 times, most recently from ebc0c63 to 98cc050 Compare April 28, 2020 15:46
@ronag ronag force-pushed the v14.x-runtime-deprecate-transformState branch from 98cc050 to 095378e Compare April 28, 2020 15:49
@ronag ronag added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 28, 2020
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@jasnell

jasnell commented Apr 28, 2020

Copy link
Copy Markdown
Member

@nodejs/releasers @nodejs/tsc ... This is one that the TSC will definitely need to weigh in on. Semver-major PR #32763 landed recently but not in 14.x. It removed the _transformState property off the stream.Transform object without a deprecation. Strictly speaking it is private API but it's been around for so long, and it has been publicly accessible for so long, that it would ideally go through a runtime deprecation. However, since runtime deprecations are technically semver-major and 14.0.0 has already been released, we would need to make an exception to land this runtime deprecation in 14.x.

Comment thread lib/_stream_transform.js Outdated
@lpinca

lpinca commented Apr 28, 2020

Copy link
Copy Markdown
Member

Should we add a test?

@ronag

ronag commented Apr 28, 2020

Copy link
Copy Markdown
Member Author

Should we add a test?

Do we do that for runtime deprecations?

@lpinca

lpinca commented Apr 28, 2020

Copy link
Copy Markdown
Member

I don't remember or I wouldn't have asked 😄

@lpinca

lpinca commented Apr 28, 2020

Copy link
Copy Markdown
Member

No, it looks like it is not needed fe069cc

@jasnell jasnell 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 but we need to make sure there are no objections from rest of @nodejs/tsc before landing.

@addaleax addaleax changed the title stream: runtime deprecate Transform._transformState [v14.x] stream: runtime deprecate Transform._transformState Apr 28, 2020
@ronag ronag force-pushed the v14.x-runtime-deprecate-transformState branch from 66b180d to c30b6cb Compare April 30, 2020 14:05
@ronag ronag requested a review from a team May 2, 2020 19:34
@Trott

Trott commented May 4, 2020

Copy link
Copy Markdown
Member

LGTM but we need to make sure there are no objections from rest of @nodejs/tsc before landing.

In effect, this is true, but as a clarification (because I think this will still come as a surprise to some TSC members):

This is not the TSC's decision to make anymore. It once was, but for well over a year now, It has been the Release WG's decision. In the Release WG's charter, the TSC specifically delegated determining the contents of a release to them. The TSC cannot override their authority on this. The TSC would need to de-charter the Release WG to overrule the Release WG's decisions.

That said, as far as I am aware, in the very few instances where this sort of thing has come up, the Release WG has always sought guidance from TSC on these sorts of things and basically asks the TSC to make the decision.

So, in effect, it is a TSC decision. But only because the Release WG has been reluctant to make the decision and explicitly defers to TSC.

There used to be a sentence in the Collaborators Guide indicating that the TSC needed to approve landing a breaking change in the Current branch, but it was removed over a year ago to remove the contradiction between the Release WG charter and the Collaborators Guide.

@Trott

Trott commented May 4, 2020

Copy link
Copy Markdown
Member

If it helps for TSC folks to be explicit about where they stand on this one:

I have no opinion as to whether this should land now or wait for 15.x. Either is fine by me, assuming Release WG is OK with it.

@ronag

ronag commented May 17, 2020

Copy link
Copy Markdown
Member Author

This is waiting for TSC guidance.

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

lgtm for the added deprecation

@BridgeAR

BridgeAR commented Jun 8, 2020

Copy link
Copy Markdown
Member

I abstain.

@nodejs/releasers PTAL

@BethGriggs

Copy link
Copy Markdown
Member

No objections - I'm okay with this change landing in v14.x.

@codebytere

Copy link
Copy Markdown
Member

@ronag could you please rebase this?

@ronag

ronag commented Jun 27, 2020

Copy link
Copy Markdown
Member Author

rebased

@ronag ronag force-pushed the v14.x-runtime-deprecate-transformState branch from c30b6cb to 99c4c0c Compare June 27, 2020 16:41
@codebytere

Copy link
Copy Markdown
Member

@ronag i think the rebase went a little sideways haha - once more and we should be good :)

Transform._transformState is removed in future version as part
of a refactoring.

Refs: nodejs#32763
Refs: nodejs#33105 (comment)
@ronag ronag force-pushed the v14.x-runtime-deprecate-transformState branch from 99c4c0c to b3a32c2 Compare June 28, 2020 07:57
@ronag

ronag commented Jun 28, 2020

Copy link
Copy Markdown
Member Author

@codebytere PTAL

@codebytere

Copy link
Copy Markdown
Member

Landed in db2d1ca

@codebytere codebytere closed this Jun 30, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
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
Transform._transformState is removed in future version as part
of a refactoring.

Refs: #32763
Refs: #33105 (comment)

Backport-PR-URL: #33126
PR-URL: #32763
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants