Skip to content

Conversation

@ronag
Copy link
Member

@ronag ronag commented Apr 15, 2020

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
@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Apr 15, 2020
@ronag ronag requested review from lpinca and mcollina April 15, 2020 12:46
@ronag
Copy link
Member Author

ronag commented Apr 15, 2020

@nodejs/streams

@vweevers
Copy link
Contributor

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

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

ronag commented Apr 15, 2020

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
Contributor

OK. In that case, LGTM.

@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member

I think we should do a full deprecation cycle first.

@ronag
Copy link
Member Author

ronag commented Apr 15, 2020

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
Member

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

@ronag
Copy link
Member Author

ronag commented Apr 15, 2020

Closing in favor of runtime deprecation for v15

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants