Skip to content

Conversation

@juliankrispel
Copy link

I tried to stick as close to the socket.io implementation as possible. Same with the tests. All the tests I've put in so far are passing. They don't cover all that the socket.io tests cover yet I'm afraid.

@ahdinosaur
Copy link

awesome pull request! this is the right place to make it.

@Marak
Copy link
Member

Marak commented Jul 5, 2013

Cool!

So this doesn't handle the sockjs dependency correctly. Adding both sockjs and socket.io as dependencies to socket means both libraries will install if a user needs websockets, which is not acceptable.

Having the notions of 'engines` or 'plugins' is a pattern we have been removing from Big. Everything should be a resource.

This would mean creating a sockjs and a socket.io resource in addition to the socket resource. There is a working example of this pattern with the persistence resource and the couchdb resource. @ahdinosaur is also working on a similar task with the auth resource and it's various auth providers.

In order to get this merged, we'll need to:

  • Create a sockjs resource
  • Create a socket.io resource
  • Refactor socket resource to use newly created resources

@juliankrispel
Copy link
Author

Aha! I was wondering about that. Sweet! I'm on it.

@juliankrispel
Copy link
Author

Whoops

…le, added tests to socket module, added resources folder to gitignore
@juliankrispel
Copy link
Author

So I've split out the sockjs and socket.io engines into separate resources. What bothered me a little bit was that I couldn't use resource.use for loading resources that aren't in npm or the resources folder (For tests). Am I missing something obvious?

For testing, instead of using .use I just required the resources like in this commented-out line.

@juliankrispel
Copy link
Author

Let me know if I refactored it right. What I wasn't entirely sure about was whether to keep the rpc-logic in the socket resource or put them into the individual resources. I've done the latter for now and the socket resource is now only really triggering .use.

@juliankrispel
Copy link
Author

@Marak as far as I remember you said that you'd like more bi-directional tests for the sockjs resource and a different name, something like socket-sockjs am I right? Unfortunately I lost that chat history, was on my phone. Could you clarify please, get into more detail about what tests you were looking for? Would love to get this done soon. Cheers

…ources, fixed sockjs test to stop at end, minor fix to socket schema, other minor edits usually JSLINT related
@ahdinosaur
Copy link

i wouldn't rename to socket-* just yet as think resource needs a better way to deal with namespacing before it makes sense, so i'll rename when it's ready.

as for bi-directional tests i would imagine that means a test where the client sends a message to the server and the server responds with another message or vise versa. play around with both ends sending and receiving.

@juliankrispel
Copy link
Author

Thanks! I'll get on that. I think it'd make sense to have a resource method called send, write or emit, which sends messages to all connections. Would you agree?

PS: Do you guys use particular JSLint settings?

[socket] reviewed code, added dependencies to socketio and sockjs resources
@ahdinosaur
Copy link

yeah, that makes sense, go for it.

JSLint config: https://github.com/bigcompany/big/blob/master/.jshintrc

@juliankrispel
Copy link
Author

@ahdinosaur Oh stunning didn't see that you had posted that link. I haven't been able to do anything last week but I've fleshed most of what I had in mind for sockets just now. Not yet completely finished though, I just pushed these commits to back them up, will let you know when it's done so you can review. Many thanks.

@juliankrispel
Copy link
Author

Ok. Sorry for the ridiculous delay. I had to get a new job. I've checked it over and linted. If you could review and merge that'd be fab :)

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