Adopt a stream interface for the socket instance #73

Closed
3rd-Eden opened this Issue Sep 2, 2012 · 14 comments

Comments

Projects
None yet
5 participants
@3rd-Eden
Contributor

3rd-Eden commented Sep 2, 2012

Consider implementing a stream compatible interface on top of the socket. It is already inherting from EventEmitter so making it a writeable, and readable Stream (http://nodejs.org/api/stream.html) would be a small but potentially important step as it would allow us to write pipe-able interfaces on top of engine.io.

@rauchg

This comment has been minimized.

Show comment Hide comment
@rauchg

rauchg Sep 2, 2012

Contributor

Yes

Contributor

rauchg commented Sep 2, 2012

Yes

@owenb

This comment has been minimized.

Show comment Hide comment
@owenb

owenb Oct 9, 2012

Hey Arnout

I recall you tweeting about this last week. Has the code made it into master yet, or were you proposing an API?

It would be great to have this.

owenb commented Oct 9, 2012

Hey Arnout

I recall you tweeting about this last week. Has the code made it into master yet, or were you proposing an API?

It would be great to have this.

@3rd-Eden

This comment has been minimized.

Show comment Hide comment
@3rd-Eden

3rd-Eden Oct 9, 2012

Contributor

@owenb I worked on this in my stream branch: https://github.com/3rd-Eden/engine.io/tree/stream. The only that my small test reached 100% cpu and started leaking memory.. So I'm still debugging it, but it's coming. I hope that i can finish it before realtime conf starts ;p

Contributor

3rd-Eden commented Oct 9, 2012

@owenb I worked on this in my stream branch: https://github.com/3rd-Eden/engine.io/tree/stream. The only that my small test reached 100% cpu and started leaking memory.. So I'm still debugging it, but it's coming. I hope that i can finish it before realtime conf starts ;p

@owenb

This comment has been minimized.

Show comment Hide comment
@owenb

owenb Oct 10, 2012

Great news :) I will hopefully be able to try out your branch before NodeDublin. Thanks!

owenb commented Oct 10, 2012

Great news :) I will hopefully be able to try out your branch before NodeDublin. Thanks!

@juliangruber

This comment has been minimized.

Show comment Hide comment
@juliangruber

juliangruber Nov 6, 2012

@3rd-Eden maybe have a look at https://github.com/substack/shoe for a working implementation based on sockjs which should be easy to adapt

@3rd-Eden maybe have a look at https://github.com/substack/shoe for a working implementation based on sockjs which should be easy to adapt

@3rd-Eden

This comment has been minimized.

Show comment Hide comment
@3rd-Eden

3rd-Eden Nov 7, 2012

Contributor

@juliangruber it's already working, just need to find time to add tests to it.. See https://github.com/3rd-Eden/engine.io/tree/stream

Contributor

3rd-Eden commented Nov 7, 2012

@juliangruber it's already working, just need to find time to add tests to it.. See https://github.com/3rd-Eden/engine.io/tree/stream

@juliangruber

This comment has been minimized.

Show comment Hide comment
@juliangruber

This comment has been minimized.

Show comment Hide comment
@juliangruber

juliangruber Nov 28, 2012

@3rd-Eden I see it still emits 'message' instead of 'data'. So you won't be able to pipe!

@3rd-Eden I see it still emits 'message' instead of 'data'. So you won't be able to pipe!

@3rd-Eden

This comment has been minimized.

Show comment Hide comment
@3rd-Eden

3rd-Eden Nov 28, 2012

Contributor

@juliangruber That is correct, because it's a writeable stream

Contributor

3rd-Eden commented Nov 28, 2012

@juliangruber That is correct, because it's a writeable stream

@juliangruber

This comment has been minimized.

Show comment Hide comment
@juliangruber

juliangruber Nov 28, 2012

In your code the stream is readable...and why not make it emit message and data?

In your code the stream is readable...and why not make it emit message and data?

@3rd-Eden

This comment has been minimized.

Show comment Hide comment
@3rd-Eden

3rd-Eden Nov 28, 2012

Contributor

@juliangruber would work i guess

Contributor

3rd-Eden commented Nov 28, 2012

@juliangruber would work i guess

@juliangruber

This comment has been minimized.

Show comment Hide comment
@juliangruber

juliangruber Nov 28, 2012

Hm I don't really get why you would implement only half of the stream interface, what is the win?

Hm I don't really get why you would implement only half of the stream interface, what is the win?

@Raynos

This comment has been minimized.

Show comment Hide comment
@Raynos

Raynos Dec 3, 2012

Contributor

100% Node.JS core style

You can't claim to use node.js core style if it's not a stream.

Contributor

Raynos commented Dec 3, 2012

100% Node.JS core style

You can't claim to use node.js core style if it's not a stream.

@Raynos

This comment has been minimized.

Show comment Hide comment
@Raynos

Raynos Dec 7, 2012

Contributor

image

https://github.com/Raynos/engine.io-stream

Feel free to port or inline that into engine.io. Or tell people that want a stream to just use that.

Contributor

Raynos commented Dec 7, 2012

image

https://github.com/Raynos/engine.io-stream

Feel free to port or inline that into engine.io. Or tell people that want a stream to just use that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment