Repository navigation
Deprecating autoDestroy: false and autoClose: false #30621
Description
Activity
It is not always trivial to implement streams and these options help. For example I use
autoDestroyandemitClosein ws https://github.com/websockets/ws/blob/7.2.0/lib/stream.js#L67-L68. Sometimes you need to change default behavior if it does not fit your needs.I'm fine with changing defaults but not deprecating the options.
I’m -1 in deprecating, but + 1 in getting a semver-major PR that flip those defaults.
For example I use autoDestroy and emitClose in ws websockets/ws:lib/stream.js@7.2.0#L67-L68
I'm not convinced you need those options to implement that, e.g. https://github.com/proxy/gist.github.com/ronag/3fd8012fb0b0e23114714d1629c5a0a8. Probably doesn't work but just an example.
It's probably a bit to involved of an example for me to be able to argue my point without researching a some more into it.
I don't know, I don't currently have the time to think about all the edge cases solved in that implementation and verify that they also work as expected in your example, but another reason for which those options are used is that they allow me to make the stream work in the same way in all supported Node.js versions without using
readable-stream.The point here is not to re-write lots of existing code. My main point here is how to make future stream implementers not use the options unless strictly necessary (which should be very unusual).
I don't think just changing the defaults is enough to stop people from using these options as crutches. I guess an alternative to deprecation is to add explicit discouragement in the docs?
Let’s start with changing the defaults, I think deprecating might come 1 major after that, and we are already in Node 15 territory.
Reacted by Robert NagyI don't think just changing the defaults is enough to stop people from using these options as crutches.
Maybe it's just me but I don't change defaults unless I have a good reason to do that so I don't think we should ever deprecate these options.
I mean, if I'm changing a default I know exactly what I'm doing and why I'm doing it but ofc I can't speak for other people.
I'm not convinced. I maintain that we should look into eventually deprecating them in the future. If I encounter any good example where they are indeed required I will of course reconsider. I don't think there is any hurry to actually remove them from the code. I'm mostly interested in the docs on this specific point.
I think you are contradicting yourself. On one side you say we won’t be able to port http2 to autoDestroy, and on the other that you’d like to deprecate or remove that option.
IMHO flipping the default is enough, and possibly doc-deprecation would be enough.
Reacted by Luigi PincaI think you are contradicting yourself. On one side you say we won’t be able to port http2 to autoDestroy, and on the other that you’d like to deprecate or remove that option.
I said it will be difficult and I don't think we can do it for 14.
I'm only arguing for doc-deprecation at this point, so I think we are in agreement.
Would it be an option to change the defaults for these to true and documentation deprecating them
- added a commit that references this issue
on Jan 3, 2020 How do I check whether a stream was closed due explicit
destroy()call orautoDestroy?How do I check whether a stream was closed due explicit destroy() call or autoDestroy?
You can't. Why do you want this?
For testing, where I was assuming I'd get
closeorendbut not both. It's fine, I'll share the same handler and only-once it.I was assuming I'd get close or end but not both.
This is an incorrect assumption :). You should always get
'close'.Only with #30623, right? I'm trying to get ahead of my tests failing.
Only with #30623, right? I'm trying to get ahead of my tests failing.
If you don't have
autoDestroy: trueyou should calldestroy()/destroy(err)yourself and thus'close'should be emitted. But yea, if you don't do that then it won't be emitted, at all.We've changed the default to
true. I think this is mostly resolved through that.If you don't have
autoDestroy: trueyou should calldestroy()/destroy(err)yourself and thus'close'should be emitted. But yea, if you don't do that then it won't be emitted, at all.@ronag This is difficult to apply correctly to readables. If you call
destroy()afterpush(null), it will immediately mark the stream destroyed, which means that anything that is still in the internal buffers can be discarded, depending on how the stream is consumed. To make it work, you have to listen to your own'end'emit to time it.On one hand I'm mostly supportive of the change because it makes sense, on the other hand this small change produced a subtle bug in my code, probably because I was using the stream API incorrectly :-) I wonder if that should be made somewhat more visible in a "Changes that may break your code" section in the v14 changelog.
For example I like how the folks from Flow write their changelogs (see https://github.com/facebook/flow/releases/tag/v0.125.0 as an example).
I find allowing the option of not having
autoDestroyandautoCloseas problematic and I'm not sure what the motivation for these options would be for "correctly" implemented streams.Would it be an option to change the defaults for these to
trueand documentation deprecating them? In order to make this transition faster we should explicitly set them tofalsein the places in core which are not yet updated for these.From my experience when people try to use these options it's often due not fully understanding the streams API and trying to get around that but at the same time ending up accidentally creating some very subtle bugs.
@mcollina: Thoughts?