Skip to content
New issue

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

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

Already on GitHub? # to your account

Drop readable-streams dependency #105

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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants