Conversation
Take 2, with extra close emit, and additional testing
cjihrig
left a comment
cjihrig
left a comment
There was a problem hiding this comment.
LGTM. The CI seems happy, and I tested this locally in hapi (both Node 12 and 14), and the previously seen failures appear to be resolved. I've requested reviews from a few other people.
devinivy
left a comment
devinivy
left a comment
There was a problem hiding this comment.
This looks good— I like that it carefully toes the line between leveraging real stream behavior while still allowing us to simulate contrived stream behaviors. I like having consistent autoDestroy behavior across node versions too, although I could also see merit in preserving weirdnesses for the purposes of ensuring those cases are covered by consumers of shot.
nlf
left a comment
nlf
left a comment
There was a problem hiding this comment.
this looks great! thanks for the second look, this looks ready to ship
Co-authored-by: Alex Potsides <alex@achingbrain.net>
|
Thanks for your work @kanongil and everyone's careful review. |
* fix: do not override destroy method The node docs say: > Implementors should not override this method, but instead implement readable._destroy() Fixes hapijs/hapi#4149 * do not autodestroy request stream * restore destroy method * Revert changes in favor of #133, keeping new test Co-authored-by: Alex Potsides <alex@achingbrain.net> Co-authored-by: devin ivy <devin@bigroomstudios.com>
5 participants
This version is mostly identical to the old, except that it will now emit an out-of-spec 'close' event (ie. without a preceding 'end' or 'error' emit), when requested.
Additionally, the
erroremit, is now handled through callingdestroy(), which is the correct way, and ensures that it respects theemitClose: trueoption.This should behave identically to the existing implementation, except that it fixes the bug in hapijs/hapi#4149, and another exposed in 13a94e2, where 'close' & 'end' are emitted in the wrong order.