-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
stream: remove unused & undocumented cb #32865
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
Conversation
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
|
@nodejs/streams |
|
I would prefer we add this to the public API. How else can you do asynchronous cleanup? |
In it's current form it's broken and a correct implementation would pretty much be a shorthand for the example below:
finished(stream, onDestroyed);
stream.destroy();I'm not opposed to that either. But I think we should either fix it or remove it. |
|
OK. In that case, LGTM. |
|
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? |
|
I mean, do a runtime deprecation warning in v15 and remove in v16. |
|
Closing in favor of runtime deprecation for v15 |
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), orvcbuild test(Windows) passes