search

SocketCluster / socketcluster-client

JavaScript client for SocketCluster
MIT License
290 stars 92 forks source link

Restrict simultaneous usage of host/hostname/port options fixes #105 #110

Closed MegaGM closed 6 years ago

MegaGM commented 6 years ago

Otherwise hostname is undefined if you run the client via Node.js

MegaGM commented 6 years ago

The only one usecase of port option without hostname option, which I see atm for Node.js client, is when you're in dev environment. So I guess we could restrict simultaneous usage of host/hostname/port options with no issues.

  1. We either will use hostname + port, or host alone. Not together. Then we could compute host once only, inside .create(). It will allow us to get rid of computing host inline in some functions like there https://github.com/SocketCluster/socketcluster-client/blob/master/lib/scsocketcreator.js#L28-L31 hence simplify socketcluster-client codebase abit. I like this approach, won't it break something?

  2. From the other hand, thinking about simplicity for users, the client has a default value for hostname option, the same hostname as in current URI. What if we simply set localhost as the default value for Node.js? Otherwise you'll be able to subscribe, using port without hostname when in a browser, but won't be able do the same in Node.js

I'll propose changes for 1 in meantime.

jondubois commented 6 years ago

@MegaGM OK, you convinced me. I think I prefer your solution 2 now - To use localhost as the default. What do you think?

Sometimes people use the client to communicate with SC from a different application running on the same host so that would be a moderately common use case. And yes consistency between client and server is nice.

MegaGM commented 6 years ago

Let's start with 2. Then I'll push some changes for 1.

MegaGM commented 6 years ago

Is it okay to check the hostname:port that way? If you want to merge, I'll change the PR's title. If you don't, I'll revert the last commit and change title anyway :smile_cat: