4ib3r / StompBrokerJS

NodeJS StompBroker
MIT License
36 stars 28 forks source link

Introduce adapter for different websocket implementations #8

Closed ypetya closed 6 years ago

ypetya commented 6 years ago
ypetya commented 6 years ago

The following example shows how to configure StompServer for SockJs:

const stompServer = new StompServer({ server: server, debug: console.log, path: '/ws', protocol: 'sockjs' });

console.log(' [*] Listening on 0.0.0.0:3001');
server.listen(3001, 'localhost');
stompServer.subscribe("**", stompHandler.bind(null, stompServer));

It is maybe a good idea to keep the repository clean from dependencies. Socket libraries could be injected via options.

ypetya commented 6 years ago

May I get a review for my changes please?

marverix commented 6 years ago

Do we need SockJS? What are the use cases? I'm not familliar so I would be thankful for some comments

ypetya commented 6 years ago

No, basically the StompBrokerJS repository should not be aware of the websocket wrapper implementation below. Being the broker is a bit higher level. It should work with different websocket implementations as well. Currently 'ws' or 'sockJS' as well. ( We could make it work for more later, like socket.io )

I am using this repo to be compatible with a Spring based java back-end which uses SockJs for transport. It differs from the hard-coded implementation on master branch, which uses the library 'ws'. I introduced an adapter to make it possible to go with other implementations, and added the sockJs way.

In my opinion we could eliminate these dependencies overall, by providing our interface to bind.

marverix commented 6 years ago

Nice! Thanks for explanation.

4ib3r commented 6 years ago

package-lock.json is auto-generated, so it should be added to.gitignore At My the point of view SockJs is often used and it may be in the main repo but server must run without this by default (Should we use optionalDependencies section in package.json?) Other implementations should be in their own packages.

ypetya commented 6 years ago

@marverix : About the conflicts: your implementation contains both the client and server side heart-beats. I tried to accomplish only the client side, and keep up only one timer to check all the connections timeout. I thought it would be more efficient, than creating an interval per connection. However I am not sure about this. I did not do measurements. As I consider this repository a non-production from our side yet, your implementation seems to be more complete regarding the heart-beating feature. I am good to go with yours first! I think it would be more clean to remove heart-beating functionality from this PR. Factoring out the solution from the code, can come later, so just go ahead with yours! ;)

@4ib3r : I agree, we can ignore package-lock as we do not have too many dependencies or a large project, I will fix this. (Although package-lock.json is considered to be committed into the repository, for using the exact versions: See https://docs.npmjs.com/files/package-lock.json )

The server runs with ws by default. No need to change the config, it is backwards compatible. You can find two examples in the examples folder.

I'll squash and update this PR.

ypetya commented 6 years ago

Can you please take a look on this?

ypetya commented 6 years ago

+1