fix: promise connection typescript mismatched to underlying code #3321
Conversation
This fixes a number of small type errors compared to the underlying code. This is likely a breaking change for some users as it would have required some working around. Should fix sidorares#3238 and sidorares#2667
This comment was marked as off-topic.
This comment was marked as off-topic.
Mistakenly didn't use the promise execute and query import.
| @@ -431,4 +431,7 @@ declare class Connection extends QueryableBase(ExecutableBase(EventEmitter)) { | |||
| sequenceId: number; | |||
| } | |||
|
|
|||
| export { Connection }; | |||
| declare class Connection extends BaseConnection { | |||
There was a problem hiding this comment.
Changed this to fix the pool connection inheritance giving a type error on reimplementing the promise
| on(event: 'connection', listener: (connection: PoolConnection) => any): this; | ||
| on(event: 'acquire', listener: (connection: PoolConnection) => any): this; | ||
| on(event: 'release', listener: (connection: PoolConnection) => any): this; | ||
| on(event: 'connection', listener: (connection: CorePoolConnection) => any): this; |
There was a problem hiding this comment.
This is reflective of the actual implementation.
| } | ||
|
|
||
| export interface Pool extends Connection { | ||
| export interface Pool extends QueryableAndExecutableBase, Pick<CorePool, 'escape' | 'escapeId' | 'format'> { |
There was a problem hiding this comment.
Pool doesn't extend Connection in code, so this better reflects the actual implementation
| @@ -42,6 +44,8 @@ export interface PreparedStatementInfo { | |||
| export interface Connection extends QueryableAndExecutableBase { | |||
| config: ConnectionOptions; | |||
|
|
|||
| connection: CoreConnection; | |||
There was a problem hiding this comment.
This connection field is defined on the promise Connection object, so this is now available everywhere this connection is used (including pool connection)
| @@ -63,6 +63,13 @@ declare class Pool extends QueryableBase(ExecutableBase(EventEmitter)) { | |||
|
|
|||
| promise(promiseImpl?: PromiseConstructor): PromisePool; | |||
|
|
|||
| format(sql: string, values?: any | any[] | { [param: string]: any }): string; | |||
There was a problem hiding this comment.
Copied from connection type, as these exist here in code.
| import { Pool as PromisePool } from '../../../promise.js'; | ||
|
|
||
| declare class PoolConnection extends Connection { | ||
| declare class PoolConnection extends BaseConnection { |
There was a problem hiding this comment.
Fixed inheritance type error for the promise function implementation.
|
Thanks, @ashleybartlett! The inheritance from base connections after #3081 makes perfect sense, but I'll keep the #3273 issue as related due to an unexpected breaking change when using I'm planning to run some regression tests in your changes just to make sure we wouldn't imply an additional breaking change. |
|
I think that breaking change makes a lot of sense, as it was done under the hood, but i'm not sure if it's something that can be fixed properly. Given it wasn't breaking existing checks, I can revert that change for the base type if it's going to be a problem, and it can be revisited in a breaking change release. |
ashleybartlett
changed the title
ashleybartlett
changed the title
Revisiting this, I realised that |
This fixes a couple of type errors compared to the underlying code.
This is likely a breaking change for some users as it would have required some working around to make it work before.
Should fix #3238 and #2667
Some future improvements that i'd like to see is the exported promise interfaces are converted to classes, to match the underlying code, but that felt like a much bigger change.