Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Oct 27, 2022

Conversation

webark
Copy link
Contributor

@webark webark commented Oct 6, 2022

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 webark requested a review from a team as a code owner October 6, 2022 00:33
lib/parse.js Outdated Show resolved Hide resolved
@webark
Copy link
Contributor Author

webark commented Oct 6, 2022

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

@webark webark force-pushed the fix/properly-emitting-close-event branch from dd46841 to 096dc4d Compare October 13, 2022 06:10
@webark
Copy link
Contributor Author

webark commented Oct 13, 2022

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

@webark
Copy link
Contributor Author

webark commented Oct 20, 2022

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

@lukekarrys lukekarrys merged commit 57493ee into isaacs:main Oct 27, 2022
25 checks passed
@lukekarrys
Copy link
Contributor

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.

@github-actions github-actions bot mentioned this pull request Oct 27, 2022
@webark webark deleted the fix/properly-emitting-close-event branch October 27, 2022 01:48
@webark
Copy link
Contributor Author

webark commented Oct 27, 2022

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

@bergundy
Copy link

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!

@webark
Copy link
Contributor Author

webark commented Oct 27, 2022

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

@bergundy
Copy link

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

@bergundy
Copy link

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

@webark
Copy link
Contributor Author

webark commented Oct 27, 2022

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.

lukekarrys added a commit that referenced this pull request Oct 27, 2022
I was able to replicate the same behavior from #332 by piping a readable
stream with a `highWaterMark` of 1 to extract. I confirmed this test
still failed without the fixes from #332 and now passes.
lukekarrys added a commit that referenced this pull request Oct 27, 2022
I was able to replicate the same behavior from #332 by piping a readable
stream with a `highWaterMark` of 1 to extract. I confirmed this test
still failed without the fixes from #332 and now passes.
@bergundy
Copy link

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

lukekarrys added a commit that referenced this pull request Oct 29, 2022
I was able to replicate the same behavior from #332 by piping a readable
stream with a `highWaterMark` of 1 to extract. I confirmed this test
still failed without the fixes from #332 and now passes.
lukekarrys added a commit that referenced this pull request Oct 29, 2022
I was able to replicate the same behavior from #332 by piping a readable
stream with a `highWaterMark` of 1 to extract. I confirmed this test
still failed without the fixes from #332 and now passes.
lukekarrys added a commit that referenced this pull request Oct 29, 2022
I was able to replicate the same behavior from #332 by piping a readable
stream with a `highWaterMark` of 1 to extract. I confirmed this test
still failed without the fixes from #332 and now passes.
@lukekarrys
Copy link
Contributor

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Pipeline stream unexpectly being closed when using extract
4 participants