search

Rantanen / node-mumble

Mumble client in Node.js
MIT License
155 stars 48 forks source link

Decouple module #86

Closed mikemimik closed 8 years ago

mikemimik commented 8 years ago

The module now exposes another class called ConnectionManager which is instantiated with the url and options arguments. The instantiated object has a function connect(callback(err, client)) which is the same as the usage before.

This addresses the question asked in #85

mikemimik commented 8 years ago

I think the reason the tests are failing is because of some context issues.

Rantanen commented 8 years ago

Looks good!

You're right about the tests. I'm doing some heavy database operation on the server, which is slowing everything to halt. Sorry for that confusion.

The lack of mumble.connect() is an issue though. Instead of changing the test utils, the correct solution would be to expose the connect() method in index.js like before. As stated in the discussion in the issue, we can't change the front facing API by removing methods that previously existed as that will break compatibility with the old code that uses those methods.

I'm implementing that at the moment and will get this merged in. Also moved the ConnectionManager to its own file given it's an encapsulated class of its own.

mikemimik commented 8 years ago

Yeah I understand what you mean. Would you like me to make those changes to the branch or are you going to fix that all up?

Rantanen commented 8 years ago

Oh, sorry! I said I was implementing this at the moment. :)

Also did a small breaking change (for your code): I renamed the ConnectionManager to MumbleConnectionManager. I'm expecting most users to require the class using:

var ConnectionManager = require( 'mumble' ).ConnectionManager

I felt having new MumbleConnectionManager() in the code 500 lines down would be clearer to the reader than just having new ConnectionManager() which is a bit generic. :)

Rantanen commented 8 years ago

I rolled the original 8 commits into one and merged that in as 081d6271b4f10a0aeb0604247cae0da3b2ba79bb. The functionality should now be in npm version 0.3.9.

Also for future reference, I added details about running the tests into the README. It helps to be able to execute those locally to catch the typoes, etc. :)

Thanks for the help!

(Also if you would, go ahead and send a PR to the AUTHORS file as well.)

mikemimik commented 8 years ago

I've re-implemented the module in my project and everything is working :)

I'll submit a PR with my name for the AUTHORS file. Thanks!