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.

Sign up for GitHub

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

Jump to bottom

Change to use stream.ToArray #479

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Change to use stream.ToArray #479

wants to merge 2 commits into from

Conversation

andrew-welker
Copy link

@andrew-welker andrew-welker commented Jun 21, 2023 •
edited by travi

According to issue sindresorhus/get-stream#50, there are issues with using get-stream when using node versions

= 18.14.0 and >=19.5.0. The method now used is taken directly
from the get-stream readme, and makes sense in this relatively simple scenario.

Should close #459

@andrew-welker
fix: change to use stream.ToArray
6872990 
According to issue semantic-release#50 in the `sindresorhus/get-stream` array,
there are issues with using get-stream when using node versions
>= 18.14.0 and >=19.5.0. The method now used is taken directly
from the `get-stream` readme, and makes sense in this relatively
simple scenario.
@andrew-welker
chore: remove debug statement
525e557
@andrew-welker
Copy link
Author

Looks like I need to update/modify a test? I'm not that familiar with ava as a testing framework. One of the tests is throwing an exception: stream.toArray is not a function, and I'm guessing it's because of the writerDouble in the test. Any help or insight is appreciated.

@gr2m gr2m requested a review from travi June 21, 2023 21:45
@travi
Copy link
Member

travi commented Jun 22, 2023

pulling in some of the related details:

@travi
Copy link
Member

travi commented Jun 22, 2023

thanks for the investigation and PR suggestion, @andrew-welker!

given that this was a bug in node that has since been fixed, and not a problem with get-stream or our use of it, i'm wondering if it is truly worth us changing our usage here specifically related to #459 vs simply recommending updating to the latest node versions as the proper fix.

are there details that i'm overlooking that would require this update if using the fixed node versions?

@travi travi mentioned this pull request Jun 30, 2023
Open
@bradennapier
Copy link

I am using node 20 and still have the issue as mentioned in #459

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@travi travi Awaiting requested review from travi

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Plugin ends up abruptly without completition or error, making other plugins not to start
3 participants
@andrew-welker @travi @bradennapier