Skip to content

stream: remove unused & undocumented cb - #32865

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:remove-unused-cb-api
Closed

stream: remove unused & undocumented cb#32865
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:remove-unused-cb-api

Conversation

@ronag

@ronagronag commented Apr 15, 2020

Copy link
Copy Markdown
Member

destroy(err, cb) was an undocumented API which
was previously used internally by core modules.
However, this is no longer the case and it should
be possible to safely remove this.

Refs: #32808

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

destroy(err, cb) was an undocumented API which
was previously used internally by core modules.
However, this is no longer the case and it should
be possible to safely remove this.
Refs: nodejs#32808
@ronagronag added the stream Issues and PRs related to the stream subsystem. label Apr 15, 2020
@ronag
ronag requested review from lpinca and mcollinaApril 15, 2020 12:46
@ronag

Copy link
Copy Markdown
MemberAuthor

@nodejs/streams

@vweevers

Copy link
Copy Markdown
Contributor

I would prefer we add this to the public API. How else can you do asynchronous cleanup?

@ronagronag added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 15, 2020
@ronag

ronag commented Apr 15, 2020

Copy link
Copy Markdown
MemberAuthor

I would prefer we add this to the public API.

In it's current form it's broken and a correct implementation would pretty much be a shorthand for the example below:

How else can you do asynchronous cleanup?

finished(stream,onDestroyed);stream.destroy();

I'm not opposed to that either. But I think we should either fix it or remove it.

@vweevers

Copy link
Copy Markdown
Contributor

OK. In that case, LGTM.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@mcollina

Copy link
Copy Markdown
Member

I think we should do a full deprecation cycle first.

@ronag

ronag commented Apr 15, 2020

Copy link
Copy Markdown
MemberAuthor

I think we should do a full deprecation cycle first.

Sounds good to me. Close this for now an re-open 15.x:ish? Should we add a runtime deprecation warning?

@mcollina

Copy link
Copy Markdown
Member

I mean, do a runtime deprecation warning in v15 and remove in v16.

@ronag

Copy link
Copy Markdown
MemberAuthor

Closing in favor of runtime deprecation for v15

@ronagronag closed this Apr 15, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-majorPRs that contain breaking changes and should be released in the next major version.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

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