Repository navigation
Promise version of streams.finished not calling clean up #44556
Description
node/lib/internal/streams/end-of-stream.js
Lines 245 to 255 in f0cf100
Should the promise version of finished call the clean up function returned from the callback version?
So something like:
function finished(stream, opts) {
return new Promise((resolve, reject) => {
const cleanup = eos(stream, opts, (err) => {
cleanup();
if (err) {
reject(err);
} else {
resolve();
}
});
});
}
Activity
[-]Promise version of finished not calling clean up[/-][+]Promise version of streams.finished not calling clean up[/+][-]Promise version of streams.finished not calling clean up[/-][+]Promise version of `streams.finished` not calling clean up[/+]mcollina commentedon Sep 7, 2022
cc @nodejs/streams
mcollina commentedon Sep 7, 2022
@ronag wdyt?
mcollina commentedon Sep 7, 2022
Would you like to send a PR?
rhodgkins commentedon Sep 7, 2022
Yeh can do, I've never contributed to this repo before - would tests or anything be needed or will just the above change be ok?
mcollina commentedon Sep 7, 2022
Test will be needed.
ronag commentedon Sep 28, 2022
Note that changing this semver-major. I would prefer if we added an option to the promisified version, .e.g
autoCleanup: trueor something like that which we can later change the default of if/when we want as semver-major change.tony-go commentedon Oct 2, 2022
Hey 👋
If our friend @rhodgkins finally doesn't jump on this. I would like to jump :)
Cheers
mcollina commentedon Oct 2, 2022
Go for it!
rhodgkins commentedon Oct 2, 2022
Yep sorry I'm not going to be able to do this any time soon so thanks in advance if you get time to do it! 🎉
tony-go commentedon Oct 2, 2022
It seems that @ntedgi already jump into. I'll take another one 🤓
lib: promise version of streams.finished call clean up
targos commentedon Oct 22, 2022
Fixed by #44862
lib: promise version of streams.finished call clean up
lib: promise version of streams.finished call clean up