Skip to content
This repository has been archived by the owner on Oct 31, 2018. It is now read-only.

events for closing #37

Open
shaunc opened this issue Aug 10, 2015 · 2 comments
Open

events for closing #37

shaunc opened this issue Aug 10, 2015 · 2 comments

Comments

@shaunc
Copy link

shaunc commented Aug 10, 2015

While looking into trying to create a proper PR to support callbacks on write and end I notice that you emit end and close events, but not finish events.

I understand that you don't support the (new) node stream interface. Is this intentional?

@dominictarr
Copy link
Owner

I'm happy to add a finish event, but the reason that this doesn't "support" the new interface is because this module predated was already perfect before it existed.

And regarding the new stream interface, you should also ask "which one?". "new streams" has always been backwards compatible (which made it very complicated and ugly). and if you actually go through and read the newest code, and compare it to the classic code then you'll see that how it works is basically the same.

classic streams was pretty simple. the important code was only about 100 lines. I read this code many times. .pipe hooked the 'data' and 'end' events on the source stream to the write(data) and end() methods on the destination stream. for streams two, this all worked differently. instead there was a 'readable' event, which indicated you could call the read() method and you'd get data. And there was a bunch of backwards-compatibility cruft so that if you registered a listener for 'data' it would work like a classic stream. It was thousands of lines and I thought it was obviously misguided effort, so i just kept on using the old streams, which was compatible anyway. Eventually, they realized it was too complicated, and the classic design wasn't that bad afterall and if you look at the latest code, the read() method is still in there, but it isn't used by default! it's now 'data' events, just like classic streams, but with backwards-compatibility cruft to streams2! I didn't change anything about through and it kept working just fine the entire time.

That said, there where a few streams2 changes that where good, like starting the steam as paused until it's piped, and could have easily been added to streams1 without making it thousands of lines. Also, in streams 2 the core node streams (fs, net, http, etc) where implemented around ReadableStream, an abstract class that provided most of the logic and made it relatively easy to implement a stream. Of course, we where already doing this in userland with streams1, using this module and others.

Anyway, there are still plenty of problems with streams3, (errors are not propagated, and they are still a lot of code). In the mean time i started experimenting and we (@Raynos was instrumental) came up with pull-streams which is massively simpler, supports the best features of node streams (backpressure) but also has error propagation and is more composable. This is now what I use when I write streaming code.

@shaunc
Copy link
Author

shaunc commented Aug 10, 2015

Great -- thanks so much for the info! I am relatively new to node so just started from the perspective of reading the current documentation (which isn't crystal clear :)). I will look into pull-streams... I need streams that I can use with promises -- so reliable events and/or callbacks I can wrap in a new Promise.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants