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

Improve Node.js Readable/Writable converters, introduce Duplex converter #2496

Closed
wants to merge 10 commits into from

Conversation

armanbilge
Copy link
Member

@armanbilge armanbilge commented Jul 21, 2021

Idiomatic APIs as inspired by their JVM cousins.

Edit: this PR is becoming a catch-all for other fixes.

Edit 2: okay, I ended up doing some major reworking in anticipation of new socket implementations.

@@ -134,7 +137,12 @@ private[fs2] trait ioplatform {
}.drain
}

def mkWritable[F[_]](implicit F: Async[F]): Resource[F, (Writable, Stream[F, Byte])] =
def mkWritable[F[_]: Async](f: Writable => F[Unit]): Stream[F, Byte] =
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about the name of this method, thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

readWritable? Not my favorite but matches the convention used on JVM side

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a fan myself, but I do like the consistency. Also renamed from{Readable,Writable} to readReadable and writeWritable.

Comment on lines 49 to 54
if (!readable.readableEnded)
if (!readable.readableEnded & destroyIfNotEnded)
F.delay(readable.destroy())
else
F.unit
Copy link
Member Author

Choose a reason for hiding this comment

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

Still not thrilled with this but I think it is the best we can do. The underlying issue is that Node.js has no way to close/"end" a Readable besides the nuclear option destroy. This same thing came up in #2453 (comment) when we couldn't implement the endOfInput method for sockets.

@armanbilge armanbilge changed the title Make Node.js Readable/Writable converters more idiomatic Improve Node.js Readable/Writable converters Jul 24, 2021
@armanbilge armanbilge changed the title Improve Node.js Readable/Writable converters Improve Node.js Readable/Writable converters, introduce Duplex converter Jul 25, 2021
def readWritable[F[_]: Async](f: Writable => F[Unit]): Stream[F, Byte] =
Stream.empty.through(toDuplexAndRead(f))

def toDuplexAndRead[F[_]: Async](f: Duplex => F[Unit]): Pipe[F, Byte, Byte] =
Copy link
Member Author

Choose a reason for hiding this comment

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

An even funkier name! Thoughts? 😅

@armanbilge
Copy link
Member Author

@mpilquist apologies for all the churn. In these latest commits I introduced the Duplex (i.e., readable and writable) facade along with a converter. To avoid duplicating code, I reimplemented the readable/writable converters in terms of the duplex converter although this does add quite a bit of overhead. It is very unfortunate that Node.js doesn't offer a way to create a Duplex from a Readable and a Writable.

Duplex streams are pretty important in Node.js (e.g., their sockets implement Duplex) so the idea is to use these changes to cleanup the Socket implementation in a forthcoming PR.

Thanks!

@armanbilge
Copy link
Member Author

Closing in favor of #2504 because it's easier to debug these converters alongside the socket implementations.

@armanbilge armanbilge closed this Jul 26, 2021
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