node-tar
node-tar copied to clipboard
fix: ensuring close event is emited after stream has ended.
For node 18 compatibility
Fixes #321
Node 18 pipelines specifically require the end event to be emitted and handled before the close event is.
I tried a test with a local tar file and it was still passing without the fix, but a remote tar file failed, so just stuck to that.
@jeferson-sb @lukekarrys let me know if you have any thoughts on this.
@lukekarrys squashed the commits and reworded to pass the commit lint.
@lukekarrys pinging again to see if you can approve the verifications.
Thanks for sticking with this @webark! I left a note above about a comment, but I decided I'd rather approve and merge and not risk forgetting about this (yet again).
Thanks again! This should be released as a patch within the next week.
Thank you! :) Just saw your previous comment and was about to push a change. I'll check back in a couple of months once node 18 has baked a bit and look into opening a PR to switch it to nextTick
nit: Promise.resolve().then which works on Node 10 is equivalent to queueMicrotask and cheaper than nextTick.
It's not critical, just thought I'd bring this to your attention.
Thanks everyone for putting this fix together and reviewing!
@bergundy I didn't look into the performance case of that :( Should I open another PR with the Promise approach?
It’s really not too critical imho, up to you.
Here’s more information about the difference between tasks and microtasks FTR: https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/
That's was a good read! thanks for sharing 😊
@lukekarrys Do you know how long that node 10 is going to be supported?
We could also leverage the promise conditionally depending on if the micro task is available if node 10 support is going to be around for longer than a couple of months.
I see a release PR was created by a bot: #326, can we get this approved please?
I see a release PR was created by a bot: #326, can we get this approved please?
This has been released as 6.1.12. Sorry for the delay, I wanted to do a few upstream tests in npm/cli first. Thanks to @webark for the fix and @bergundy for the review and help!