Skip to content

remove dependency readable-stream#386

Closed
Uzlopak wants to merge 5 commits intomasterfrom
remove-readable-stream
Closed

remove dependency readable-stream#386
Uzlopak wants to merge 5 commits intomasterfrom
remove-readable-stream

Conversation

@Uzlopak
Copy link
Copy Markdown
Contributor

@Uzlopak Uzlopak commented Sep 2, 2022

Checklist

@Uzlopak Uzlopak requested review from Fdawgs and mcollina September 2, 2022 10:49
@Fdawgs
Copy link
Copy Markdown
Member

Fdawgs commented Sep 4, 2022

Quick one, why does it need replacing?

@kibertoad
Copy link
Copy Markdown
Member

@Fdawgs Isn't it generally preferable to use native solutions over custom libraries?

@Uzlopak
Copy link
Copy Markdown
Contributor Author

Uzlopak commented Sep 4, 2022

I am actually trying to figure out why the tests are flaky. I have the bad feeling that it has something to do with how busboy works and the events it is calling. Something like finish instead of close. Unfortunately I can not reproduce the issues on my local machine. Something is quite odd.

@Fdawgs
Copy link
Copy Markdown
Member

Fdawgs commented Sep 4, 2022

@Fdawgs Isn't it generally preferable to use native solutions over custom libraries?

Of course! Sorry, had a brain fart and forgot stream was native. 😂

@Eomm
Copy link
Copy Markdown
Member

Eomm commented Sep 4, 2022

Unfortunately I can not reproduce the issues on my local machine.

Same error as the CI for me with node v18.6.0

@Uzlopak
Copy link
Copy Markdown
Contributor Author

Uzlopak commented Sep 4, 2022

Yes on node 18.8.0 it breaks. On node 16.17.0 it works.

So there is an issue when running it on node 18 and I have no clue.

@kibertoad
Copy link
Copy Markdown
Member

Streams notoriously change all the time across Node versions :D

@climba03003
Copy link
Copy Markdown
Member

climba03003 commented Sep 5, 2022

Isn't it generally preferable to use native solutions over custom libraries?

@kibertoad
For stream, NO. It prefer to use readable-stream for library.
If it is self project and tagged to single node version, then native is better.

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.

5 participants