oortcloud / node-ddp-client

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

Make port argument optional #14

Closed AjayMT closed 11 years ago

AjayMT commented 11 years ago

Don't force the port argument to be something like '3000'. This prevents you from having to do something weird like this to connect to a published app:

var client = DDPClient({
  host: 'myapp.meteor.com'
});
client.port = '';

Now you can just omit the port argument, which looks like this:

var client = DDPClient({
  host: 'myapp.meteor.com'
});
tmeasday commented 11 years ago

@AjayMT -- what port does setting port to '' connect to? Should we just make the default that?

sarfata commented 11 years ago

@tmeasday: I guess 80 - This would be in ws, not in node-ddp-client.

The default host is 'localhost' and for localhost it makes sense to have 3000 as the default value.

What about leaving the default host and port to 3000 - but reverting back to 80 when the user passes a parameter for the host argument? It would then be his responsibility to set the port if it is not 80 (unless use_ssl is set, in which case the port would be 443).

tmeasday commented 11 years ago

It does make sense for 3000 to be the default port for localhost, but I'm not sure it's great to have default ports changing based on host.

I'm tempted to just leave this as is and @AjayMT should just use port: 80.

Or, we just change the default port to 80.

AjayMT commented 11 years ago

Setting the port to '' appears to be the same as setting the port to 80 (and I just realized it isn't necessary to do client.port = ''). Setting the default port to 3000 is fine for development, but connecting to published apps requires that the port be 80.

So, all that this PR really does is change the default port from 3000 to 80 ('' is basically the same as 80).

tmeasday commented 11 years ago

Well I don't know what the default port should be. Probably doing what ws does is fine, but I'm also tempted to just leave it as is seeing as the point of annoyance for you is gone @AjayMT .

@sarfata deciding vote?

sarfata commented 11 years ago

If it ain't broken don't fix it ...

Connection to a local meteor server is our default and it makes sense. If you want something else, be explicit.

I vote to keep the current implementation.

Thanks a lot @AjayMT for your contribution and a great discussion!

Thomas

On 1 avr. 2013, at 13:22, Tom Coleman notifications@github.com wrote:

Well I don't know what the default port should be. Probably doing what ws does is fine, but I'm also tempted to just leave it as is seeing as the point of annoyance for you is gone @AjayMT https://github.com/AjayMT .

@sarfata https://github.com/sarfata deciding vote?

— Reply to this email directly or view it on GitHubhttps://github.com/oortcloud/node-ddp-client/pull/14#issuecomment-15714999 .