-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Consider "end" even in addition to "close" on @serialport/stream #1690
Comments
If we're keeping the Stream interface as an implementation of the Duplex stream, then Helps keep things consistent with how people learn streams, i.e. https://medium.freecodecamp.org/node-js-streams-everything-you-need-to-know-c9141306be93#a9a7 |
The bindings literally never return 0 bytes, it's the spec that they have to return at least a single byte or wait for data. The only time the stream ends is on close. |
And once a stream has ended I'm pretty sure it can't be started again. |
So is the check for 0 bytes in the stream source code just for safe measure? this.binding.read(pool, start, toRead).then(
bytesRead => {
debug('binding.read', `finished`)
// zero bytes means read means we've hit EOF? Maybe this should be an error
if (bytesRead === 0) {
debug('binding.read', 'Zero bytes read closing readable stream')
this.push(null)
return
}
pool.used += bytesRead
this.push(pool.slice(start, start + bytesRead))
},
err => {
debug('binding.read', `error`, err)
if (!err.canceled) {
this._disconnected(err)
}
this._read(bytesToRead) // prime to read more once we're reconnected
}
) |
I guess so, I don't really remember =)
|
Worked in #1926 thinking it will be an option to enable or disable this behavior. If we have it on by default we need a HUGE warning in the changelog. |
It seems unexpected to have this off by default. It means that, while Similarly, the |
💥 Proposal
From #1688 (comment) it might be worth adding the
end
event to the stream. We would do it to work well with other streams.@HipsterBrown do you think it's needed?
The text was updated successfully, but these errors were encountered: