-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix: use stream helpers #1
Conversation
proxy.setWritable(decode) | ||
proxy.setReadable(encode) | ||
eos(proxy, cleanup) | ||
const proxy = Duplex.from({ writable: decode, readable: encode }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires node 16.8.0 and probably can't be browserified either because readable-stream
is behind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although, on the browser point, perhaps it's time to let go of the old and build something web stream-based
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sad but true... to both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use Duplex.from
if available and otherwise fallback to duplexify
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things I want to do first:
- Benchmark
Duplex.from
, I didn't try that yet. And design a more suitable benchmark, because I've focused mainly on iterators so far, less on one-of operations likeput()
andget()
. And swaplevel
formemory-level
to remove fs from the equation. And bypassnet
sockets. - Check compat with
length-prefixed-stream
which usesreadable-stream
and therefore doesn't satisfy core'swillEmitClose
logic. Andlength-prefixed-stream
has its owndestroy()
😞. Not sure what the effect is though, if any. Can you help with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need some time to think about which flavor of streams to go with. It's a difficult question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would personally not use readable-stream or any of the non-core helpers in production. But that's just my opinion. IMHO They are basically unmaintained and have lots of bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indirectly that unfortunately includes any lib that uses them.
Superseded by #7. |
I've spent a lot of time sorting out problems with the eos & duplexity packages in core. Please use the core versions. 😄