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

Remove dependency with mysql-connection-manager #50

Closed
fcarreiro opened this issue Aug 31, 2016 · 5 comments
Closed

Remove dependency with mysql-connection-manager #50

fcarreiro opened this issue Aug 31, 2016 · 5 comments

Comments

@fcarreiro
Copy link
Contributor

@fcarreiro fcarreiro commented Aug 31, 2016

I think that the managing of connections is orthogonal to the objective of this module. The dependency with mysql-connection-manager could be decoupled by accepting, in the construction of the session object, either a connection (and use #query), a pool (and use #query) or a function which in turn is expected to return a connection. Then, the following would work:

var connection = mysql.createConnection(options);
var sessionStore = new MySQLStore({}/* session store options */, connection);
var pool = mysql.createPool(options);
var sessionStore = new MySQLStore({}/* session store options */, pool);
var MySQLConnectionManager = require('mysql-connection-manager');
var manager = new MySQLConnectionManager(options);
function getConnection() { return manager.connection; }
var sessionStore = new MySQLStore({}/* session store options */, getConnection);

In any case, as we have been discussing in this issue I think I'd recommend deprecating mysql-connection-manager, and this issue goes in that direction.

Actually, if the possibility of passing a MySQLConnectionManager itself was never documented, I'd just accept a connection or pool (which are "the same" if you just use #query and similar).

I am happy to change the code and submit a pull request, if you agree with this.

@chill117
Copy link
Owner

@chill117 chill117 commented Aug 31, 2016

This sounds good to me.

@fcarreiro
Copy link
Contributor Author

@fcarreiro fcarreiro commented Aug 31, 2016

With or without function support as a parameter? :)

@chill117
Copy link
Owner

@chill117 chill117 commented Aug 31, 2016

I am not so sure about the function support idea. Let's leave it out for now.

@fcarreiro
Copy link
Contributor Author

@fcarreiro fcarreiro commented Aug 31, 2016

Ok! I'll try to do this when I have some time, and get back to you.

@chill117
Copy link
Owner

@chill117 chill117 commented Aug 31, 2016

Great, thank you!

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

Successfully merging a pull request may close this issue.

None yet
2 participants