Inconsistencies with promise.js
Opened this issue · 11 comments
I haven't looked much into this, but promise.d.ts defines a Connection interface with a changeUser method, but https://github.com/sidorares/node-mysql2/blob/master/promise.js defines PromiseConnection with a different structure. I noticed this when I got a run-time error stating my Connection object had no changeUser method.
This PromiseConnection is what is returned from createConnection.
https://github.com/sidorares/node-mysql2/blob/master/promise.js does not define a createUser method, but you do have access to the underlying Connection.
Feel free to do a PR
I plan to. Would it be alright to change the name of Connection to PromiseConnection?
What would be the benefit? It would be a breaking change
config, threadId are also not defined in PromiseConnection. connection is missing. Not sure of any differences yet. I don't know the history of mysql2, but it looks like the API, at least for the promise wrapper, changed drastically at some point if I compare its current design to your type definitions.
Maybe we could leave Connection as is and also provide PromiseConnection (as its called in mysql2).
I think the promise wrapper used to be the same API as the callback API, just returning promises. Is that not the case anymore? If not, in what version was it changed?
I believe the point of the wrapper is to have the same API, but it is currently not the case. There are some things missing, some things that don't belong in you current definitions. Could you check https://github.com/sidorares/node-mysql2/blob/master/promise.js and let me know what you think? Thank you.
Not sure what you want me to look for?
The differences from your definitions. Like the examples I've given.
I haven't used MySQL in a very long time, multiple years now. I don't have time to aggregate the differences, but will happily accept a PR if you wanna bring it up-to-date
No problem. Do you want to keep the name Connection inside promise.d.ts, change it to PromiseConnection as it's called in mysql2, or keep the old definition of Connection and add PromiseConnection alongside it? (index.d.ts Connection wouldn't change in either case).
I would try to avoid breaking changes if possible, so I don't see a reason to rename it (it's just an interface and the implementation is not exported)