node-tar icon indicating copy to clipboard operation
node-tar copied to clipboard

fix: ensuring close event is emited after stream has ended.

Open webark opened this issue 3 years ago • 2 comments

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.

webark avatar Oct 06 '22 00:10 webark

@jeferson-sb @lukekarrys let me know if you have any thoughts on this.

webark avatar Oct 06 '22 00:10 webark

@lukekarrys squashed the commits and reworded to pass the commit lint.

webark avatar Oct 13 '22 06:10 webark

@lukekarrys pinging again to see if you can approve the verifications.

webark avatar Oct 20 '22 23:10 webark

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.

lukekarrys avatar Oct 27 '22 01:10 lukekarrys

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

webark avatar Oct 27 '22 01:10 webark

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 avatar Oct 27 '22 03:10 bergundy

@bergundy I didn't look into the performance case of that :( Should I open another PR with the Promise approach?

webark avatar Oct 27 '22 03:10 webark

It’s really not too critical imho, up to you.

bergundy avatar Oct 27 '22 04:10 bergundy

Here’s more information about the difference between tasks and microtasks FTR: https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/

bergundy avatar Oct 27 '22 04:10 bergundy

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.

webark avatar Oct 27 '22 09:10 webark

I see a release PR was created by a bot: #326, can we get this approved please?

bergundy avatar Oct 28 '22 20:10 bergundy

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!

lukekarrys avatar Nov 01 '22 16:11 lukekarrys