search

types / mysql2

Typings for https://github.com/sidorares/node-mysql2
ISC License
41 stars 23 forks source link

Inconsistencies with promise.js #16

Open rhyek opened 7 years ago

rhyek commented 7 years ago

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.

felixfbecker commented 7 years ago

Feel free to do a PR

rhyek commented 7 years ago

I plan to. Would it be alright to change the name of Connection to PromiseConnection?

felixfbecker commented 7 years ago

What would be the benefit? It would be a breaking change

rhyek commented 7 years ago

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).

felixfbecker commented 7 years ago

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?

rhyek commented 7 years ago

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.

felixfbecker commented 7 years ago

Not sure what you want me to look for?

rhyek commented 7 years ago

The differences from your definitions. Like the examples I've given.

felixfbecker commented 7 years ago

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

rhyek commented 7 years ago

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).

felixfbecker commented 7 years ago

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)