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.

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

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

Conversation

ronag
Copy link
Member

@ronag ronag commented Apr 28, 2020

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
Collaborator

@jasnell
Copy link
Member

jasnell commented Apr 28, 2020

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

lib/_stream_transform.js Outdated Show resolved Hide resolved
@lpinca
Copy link
Member

lpinca commented Apr 28, 2020

Should we add a test?

@ronag
Copy link
Member Author

ronag commented Apr 28, 2020

Should we add a test?

Do we do that for runtime deprecations?

@lpinca
Copy link
Member

lpinca commented Apr 28, 2020

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

@lpinca
Copy link
Member

lpinca commented Apr 28, 2020

No, it looks like it is not needed fe069cc

Copy link
Member

@jasnell jasnell left a comment

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
Copy link
Member

Trott commented May 4, 2020

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
Copy link
Member

Trott commented May 4, 2020

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
Copy link
Member Author

ronag commented May 17, 2020

This is waiting for TSC guidance.

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.

lgtm for the added deprecation

@BridgeAR
Copy link
Member

BridgeAR commented Jun 8, 2020

I abstain.

@nodejs/releasers PTAL

@BethGriggs
Copy link
Member

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

@codebytere
Copy link
Member

@ronag could you please rebase this?

@ronag
Copy link
Member Author

ronag commented Jun 27, 2020

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
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
Copy link
Member Author

ronag commented Jun 28, 2020

@codebytere PTAL

@codebytere
Copy link
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