Skip to content

Drop readable-streams dependency #105

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

Merged
merged 2 commits into from
Aug 24, 2024
Merged

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Aug 23, 2024

stream.Duplex has been available since Node 6.8.0 0.9.4

https://nodejs.org/api/stream.html#duplex-and-transform-streams

@fregante fregante mentioned this pull request Aug 23, 2024
16 tasks
@mcollina
Copy link
Member

I suspect Duplex was around from 0.10, but that's not the reason why we are using readable-stream here.

The whole point of readable-stream is to normalize the stream dependency a wide array of Node.js versions.

@mcollina
Copy link
Member

I think this PR should include a bump to a minimum supported version of node v18 (right now it's v14 for this module).

@fregante
Copy link
Contributor Author

I don’t think CI failing is due to a bump, it seems GitHub actions is just failing to download Node. Try again:

Attempting to download 14...
Not found in manifest. Falling back to download directly from Node
Error: Unable to find Node version '14' for platform darwin and architecture arm64.

@fregante
Copy link
Contributor Author

to normalize the stream dependency a wide array of Node.js versions.

I don’t know much on the topic, but the article linked on readable-streams explaining this is exactly 10 years old, I think we’re past the streams 2/3 issues: https://r.va.gg/2014/06/why-i-dont-use-nodes-core-stream-module.html

@jsumners
Copy link
Member

I don’t think CI failing is due to a bump, it seems GitHub actions is just failing to download Node. Try again:

Attempting to download 14...
Not found in manifest. Falling back to download directly from Node
Error: Unable to find Node version '14' for platform darwin and architecture arm64.

Seems like some sort of bug:

https://github.com/actions/node-versions/blob/75581b602712663c8c992deca984d30c12624e88/versions-manifest.json#L3232

Far easier to just remove 14 from the matrix.

@fregante fregante mentioned this pull request Aug 23, 2024
@mcollina mcollina merged commit d430839 into pinojs:main Aug 24, 2024
12 checks passed
@fregante fregante deleted the native-streams branch August 24, 2024 16:00
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.

3 participants