Fix https://github.com/porsager/postgres/issues/1143

[sep2]

Omit will not work, a common interface ISql for Sql and TransactionSql is introduced.

microsoft/TypeScript#41362

[porsager]

Won't that include all properties that aren't valid for TransactionSql ? I think the proper way is to make an explicit definition of TransactionSql instead of inherit. Perhaps better to use Type instead of Interface.

[sep2]

Only actual properties are included in the interfaces. Properties not in the runtime trx object are moved to sql only. The super type contains only shared properties that could be found in both runtime objects.

Object.keys(sql) [
  'types',         'typed',
  'unsafe',        'notify',
  'array',         'json',
  'file',          'parameters',
  'largeObject',   'subscribe',
  'CLOSE',         'END',
  'PostgresError', 'options',
  'reserve',       'listen',
  'begin',         'close',
  'end'
]
Object.keys(trx) [
  'types',   'typed',
  'unsafe',  'notify',
  'array',   'json',
  'file',    'savepoint',
  'prepare'
]

type could work but that's potentially a breaking change, with interface the changes are minimized.

I used sql/trx extensively with this PR in my local project, haven't found any type errors so far. Nothing in my project needs to adapt.

[mattbishop]

It would be good to add a unit test, like what is common in DefinitelyTyped.

[porsager]

To prevent the same hazard as previous PR, can some more people with typescript knowledge approve this?

[abrahamguo]

@porsager I opened the original issue (#1143), and I can confirm that this looks good to me. I've also tested this in my project, and it resolves the issue that I was seeing.

@sep2's line of thinking, of moving all shared properties (sql.types/trx.types, sql.unsafe/trx.unsafe, and so on) AND the call signature (sql()/trx()) to a new shared interface used by both sql and trx is the right approach.

[abrahamguo]

The only nitpick I would have is that since there's already an existing interface called Sql, creating a new interface ISql is a poor name, because it's unclear what the difference is between Sql and ISql.

A more descriptive name might be BaseSql.

[AceCodePt]

I would prefer the type to be unknown instead of any. The rest LGTM

[rvitaliy]

LGTM

[danieltroger]

Please merge this, the library is unusable right now on the latest version. The changes LGTM too.

[karlhorky]

cc @Minigugus @Newbie012 @thebearingedge in case you have a moment to review the types fix here

Currently in the middle of teaching a cohort of students, but I'll try to put this on my list for the coming weeks if you don't have time

[karlhorky]

To prevent the same hazard as previous PR, can some more people with typescript knowledge approve this?

@porsager to automatically verify both this types PR and also future types PRs, one option that could also be considered would be to add some types tests using a project such as:

• https://github.com/mmkal/expect-type
• https://github.com/tsdjs/tsd
• https://github.com/rauschma/asserttt
• https://github.com/JoshuaKGoldberg/eslint-plugin-expect-type

Also, if using Vitest would be an option, then it has type testing built in:

• https://vitest.dev/guide/testing-types

[AntonOfTheWoods]

@porsager are you sure you haven't seen enough people tell you it's alright to merge this now?

[porsager]

Thanks for pinging @AntonOfTheWoods - Will make a release once I'm caught up on a few other pending issues

[porsager]

Apparently not enough reviewers @AntonOfTheWoods

#1163

[AntonOfTheWoods]

Apparently not enough reviewers @AntonOfTheWoods

#1163

Yeah, we may need to get some claude action on this or something. I can say that the update fixed my issues, and probably those for many others. So not a total loss, given the person with the issue said it was also present on 3.4.8? So maybe just a small victory rather than Victory?

[porsager]

Yay - I read too fast - wrongly saw it as not present in 3.4.8, so let's go with Victory for now ;)