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

Readable streams 309 #312

Closed
wants to merge 2 commits into from
Closed

Conversation

bencoder
Copy link

@bencoder bencoder commented Sep 6, 2024

Checklist

@bencoder bencoder force-pushed the readable-streams-309 branch from 9340706 to 2c7e051 Compare September 6, 2024 09:07
@bencoder bencoder force-pushed the readable-streams-309 branch from 2c7e051 to 6294812 Compare September 6, 2024 09:11
@bencoder
Copy link
Author

bencoder commented Sep 6, 2024

First commit is working, using the workaround mentioned in issue #309, but feels a little hacky.

Second commit is attempting to switch from using pump to require('node:stream').pipeline as suggested by @mcollina in the issue, which works to solve the ReadableStream issue, but fails a bunch of the decompression tests in a way that I am struggling to follow.

as part of the second commit I also had to test the payload instance type against ReadableStream because ReadableStream does not have a pipe method, so it fell into the if statement and errored there

}
return next(null, payload)
}

if (typeof payload.pipe !== 'function') {
if (typeof payload.pipe !== 'function' && !(payload instanceof ReadableStream)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately using instanceof here would break compatibility everywhere.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I suspected as such.
I think solving the linked issue (#309) probably requires more significant re-architecture of this plugin to more modern standards (as you discussed also here #297 (comment)) which I don't have the time/ability to work on right now, so I will close this PR, though i'll leave my branch here as the test case may be useful to anyone else who might want to pick this up

@bencoder bencoder closed this Sep 6, 2024
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.

2 participants