mysql
mysql copied to clipboard
mysqljs
Add support for custom streams
Hello,
I needed to connect to a SOCKS proxy, as in #725, so I added a stream option similar to the mysql2 library. It only takes a factory function right now; I could add support for a plain stream, but that seems like a more unusual use case than typical.
I basically copied the socket connection test, because that seemed most appropriate. Let me know if you'd like me to DRY that up, or if you have feedback on anything else.
@sidorares any initial comments? Is this API compatible with the existing API in mysql2?
Yes, API is compatible.
@dgb would be good to add test of pool and try two connections (initially stream option was only "stream" and when used with pool it tried to perform handshake on a already opened connection, that's why semantics changed to be "give me please new duplex stream which represent fresh connection to server"
Looks like the test is failing on Node.js 0.6 if you can take a look into it.
Yep, I saw that failure--I'll look into it. I'll see about writing a pool test too. I don't think it'll be an issue since I'm only using a function for this option (mysql2 takes either a stream or a function), but I haven't thought too deeply about the full connection lifecycle and could be overlooking something.
Anyways, thanks for the feedback and have a good weekend!
Anyways, thanks for the feedback and have a good weekend!
You, too! In fact, I looked a bit, and I think the issue is because you are using net.connect instead of net.createConnection, which acts differently in Node.js 0.6; you can grep this module for the net.createConnection and just copy that into your test and I believe that should fix the issue.
Heya,
I updated the test to use net.createConnection and it looks like it's passing on Node.js 0.6 now.
As for testing pools goes: It looks like pools are only tested at the unit level (which uses a mock connection), and all of the integration tests are at the connection level. Shall I go ahead and add a test/integration/pool directory, or is there a reason for this/is that getting a little out of the scope of this PR?
Any word on this pull request? I'll gladly add some tests if need be, but like I said, might be outside of the scope of this patch. FWIW I figured whomever was looking at this went on vacation or something and forgot, and I kind of forgot as our API has been humming along on my fork, uneventfully (well, at least WRT the database library ;)).
