Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion for implementation of clients API #50

Closed
wants to merge 3 commits into from
Closed

Suggestion for implementation of clients API #50

wants to merge 3 commits into from

Conversation

cha0s
Copy link

@cha0s cha0s commented Jan 27, 2015

No description provided.

@cha0s
Copy link
Author

cha0s commented Jan 27, 2015

I bumped socket.io and socket.io-client in the dev dependencies, obviously they will have to be bumped again if this goes in to reflect the other PRs I've made on this topic.

@cha0s
Copy link
Author

cha0s commented Jan 27, 2015

socketio/socket.io-adapter#24
socketio/socket.io#1966

Just to link them together

@rauchg
Copy link
Contributor

rauchg commented Jan 28, 2015

So, this is a great foundation for this patch. Thanks a lot for submitting this PR.

I think it's going to require some minor refactoring to adapt to our goal of getting rid of pattern subscriptions. Otherwise, with minor potential API changes, this is the solution we'll deploy.

The API changes I'm still debating in my mind are:

  • do we care about an API to only receive local clients (to the specific node), or do we make it transparent and always expose all nodes?
  • do we care about filtering by room?

@rauchg
Copy link
Contributor

rauchg commented Jan 28, 2015

So far I think I'm No - Yes on those questions.

@rauchg
Copy link
Contributor

rauchg commented Jan 28, 2015

If the answers were Yes - Yes, I think we should consider exposing two methods clients and allClients

@cha0s
Copy link
Author

cha0s commented Jan 28, 2015

Interesting, first I saw #46. It's a really good idea to do things that way.

I think it makes sense to filter based on namespace/room. Especially once #46 goes in it'll be even less overhead since we won't have to hit every namespace in every node

@cha0s
Copy link
Author

cha0s commented Jan 28, 2015

Oh, also I'm not sure about the local/remote clients thing. I'm leaning towards No - Yes myself, because I don't think the overhead will be too high, and once you get those IDs you could check them against the local state (though, peeking into internals, which isn't polite...)

Honestly when I started this out I was envisioning being able to use a socket ID from a remote node to be able to emit a message from another node, but that seems like a convulted process as well. I almost wonder if this API should be introduced only if there is some kind of '(emit|broadcast)To' function that instead of operating on a socket, takes a socket ID. I have been struggling with the idea that perhaps the use case for clients as it stands isn't as useful as I thought. However, if enough people want it I don't mind anyway.

@rauchg
Copy link
Contributor

rauchg commented Feb 20, 2015

@cha0s the API you mention already exists. Every socket joins a room identified by their session id.
http://socket.io/docs/rooms-and-namespaces/#default-room

@cha0s
Copy link
Author

cha0s commented Feb 20, 2015

I totally forgot about that. Obviously that makes this a lot more useful than I thought when I wrote that post!

hitsujiwool added a commit to hitsujiwool/socket.io-redis that referenced this pull request Aug 28, 2015
adapted original cha0s's implementation to the latest pub-sub optimized version.

socketio#50
@darrachequesne
Copy link
Member

Closed by #109

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

Successfully merging this pull request may close these issues.

3 participants