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

feat: add end event for piping #1926

Merged
merged 1 commit into from
Sep 22, 2019
Merged

feat: add end event for piping #1926

merged 1 commit into from
Sep 22, 2019

Conversation

reconbot
Copy link
Member

This will allow streams to end. We haven't done this because it allows a stream to be reopened and data to be continued to be read. This may not be a good idea but I'm opening this issue for discussion.

Maybe we should have this as an option?

@HipsterBrown
Copy link
Contributor

I agree that if this functionality is introduced, it should be off by default and enabled by an option in the constructor params. Otherwise I would count it in a major version bump.

@reconbot
Copy link
Member Author

We got a major release in beta, unsure what the default should be. Probably best to keep it the same as it is now even if it’s dissimilar to fs and net.

@reconbot
Copy link
Member Author

Specifically because people seem to constantly open and close ports or they never close ports.

This will allow streams to end. We haven't done this because it allows a stream to be reopened and data to be continued to be read. 

Adds `endOnClose` to stream options so the stream will emit the end event when the port is closed. This will signal any piped parsers to end as well. If the port is reopened the parsers won’t function.
@reconbot
Copy link
Member Author

reconbot commented Sep 3, 2019

How's this?

@HipsterBrown
Copy link
Contributor

Looks good to me. 👍 👏

@reconbot reconbot merged commit 275315a into master Sep 22, 2019
@reconbot reconbot deleted the reconbot/end-event branch September 22, 2019 19:27
@lock lock bot locked as resolved and limited conversation to collaborators Mar 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants