GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — lib: promise version of `streams.finished` call cleanup after eos by ntedgi · Pull Request #44862 · nodejs/node Conversation
|
|
Review requested:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening a PR! Can you please add a unit test?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
i added the following tests In addition, I want to make sure that the cleanup function has been called as expected I didn't find any example of how to detect listeners has been properly removed I see it hasn't been tested here as well : https://github.com/nodejs/node/blob/main/test/parallel/test-stream-end-of-streams.js Can you please help me think of a simple way to check |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Thank you anyway, I was able to find a simple solution |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
streams.finished call clean upstreams.finished call clean up
implement autoCleanup logic. update docs add autoCleanup description ref: #44556
doc: update autoCleanup default value to false
streams.finished call clean upstreams.finished call cleanup after eos
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| { | ||
| const streamObj = new Writable(); | ||
| assert.strictEqual(streamObj.listenerCount('end'), 0); | ||
| finished(streamObj, { cleanup: true }).then(() => { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Nitpic: wrap with common.mustCall
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| { | ||
| const streamObj = new Writable(); | ||
| assert.strictEqual(streamObj.listenerCount('end'), 0); | ||
| finished(streamObj).then(() => { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Same
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Landed in 84064bf |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
implement autoCleanup logic. update docs add autoCleanup description ref: #44556 PR-URL: #44862 Refs: #44556 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
implement autoCleanup logic. update docs add autoCleanup description ref: #44556 PR-URL: #44862 Refs: #44556 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
implement autoCleanup logic. update docs add autoCleanup description ref: #44556 PR-URL: #44862 Refs: #44556 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
implement autoCleanup logic. update docs add autoCleanup description ref: #44556 PR-URL: #44862 Refs: #44556 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
PR-URL: #55043 Refs: #44862 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: #55043 Refs: #44862 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.