oortcloud / node-ddp-client

A callback style DDP (Meteor's Distributed Data Protocol) node client.
Other
263 stars 80 forks source link

SockJs info support #51

Closed arunoda closed 9 years ago

arunoda commented 9 years ago

This PR added basic SockJs support. Actually, this still uses WebSockets. But this allows to hit the /sockjs/info first and try to support base_url property if it's there.

With this, we can use this with meteorhacks:cluster where it supports load balancing and service discovery. These features now available with this PR.

This PR is backward compatible

We need to add useSockJs option to turn on sockjs support.

var client = new DDPClient({ useSockJs: true });

Additionally, now we don't set the path as "websocket" by default. Instead it'll be set when the websocket URL is generating. meteorhacks:cluster uses path to discover services when enabled sockjs mode. So, that's the reason we keep path option empty if it is.

I'll be great, if you can have this PR :)

emgee3 commented 9 years ago

@arunoda, this looks good and I'm fine on merging this, but I'd like a few changes first:

arunoda commented 9 years ago

Yes. I'll do these now. This is not the style we code. So, I missed that :)

emgee3 commented 9 years ago

No worries

arunoda commented 9 years ago

Done. We are using this for some production stuff now. After sometimes later, we could use the sockJS mode by default.

emgee3 commented 9 years ago

Merged and published.

SockJS support by default is a good idea. I have a couple other API changes I'd like to do before a 1.0 release, and we can make it by default then.

arunoda commented 9 years ago

great and thanks.

On Wed Feb 04 2015 at 8:21:14 AM emgee3 notifications@github.com wrote:

Merged and published.

SockJS support by default is a good idea. I have a couple other API changes I'd like to do before a 1.0 release, and we can make it by default then.

— Reply to this email directly or view it on GitHub https://github.com/oortcloud/node-ddp-client/pull/51#issuecomment-72781138 .

Tarang commented 9 years ago

Is Sockjs part of the DDP Spec? If it is it may not be the best idea to make it default.

I know there's a C# DDP Server out there and a node ddp server i'm working on at the moment. If its on by default these two library wouldn't be as easy to link up.

In addition to that there's also a latency argument it slows down the connection time. I guess it depends a bit more on how this library is used.

arunoda commented 9 years ago

it's not a part of the DDP spec. That's how browsers trying to connect from the client. I agree with tarang. This should not be the default.

Anyway, DDP has no information about the transport machanism. I think having something like /info hit is great thing. This is available in engine.io as well.

Tarang commented 9 years ago

@arunoda I've not yet dived down into the meteorhacks:cluster code yet, is it possible for it to load balance without calling /sockjs? Is it something you're envisioning to load balance the websocket uri's as well as proxy the requests between the servers using the same ws uri?

arunoda commented 9 years ago

theoretically it can. That's the default mode. Then this is very similar to haproxy, nginx and other proxies. Traffic routes with a single server.

But we've another mode, which makes multiple instances to be act as load balancers. With that we can distribute the traffic very effectively. That's done with the /sockjs/info hit.

But, if you are discovering connections on the server side, then it does not use /sockjs/info hit.