slonik: QueryResultRowColumnType is too tight to use .oneFirst() on a SELECT EXISTS(...) query

[vogon]

Authors: @sebald @mmkal

in my code, I use a query like:

return this.db.oneFirst(
    sql`select exists(select 1 from permission_grants
    where user_id = ${userId} and permission = ${permission})`
);

to get a simple true-or-false answer about whether any rows match the condition. this method returns a boolean value with the default set of type parsers. however, QueryResultRowColumnType is defined as string | number, so I have to cast around it with something like:

return ((this.db.oneFirst(
    sql`select exists(select 1 from permission_grants
    where user_id = ${userId} and permission = ${permission})`
)) as string | number | boolean) as boolean;

not sure if just loosening the definition to string | number | boolean is a good idea, wanted to put this out for feedback before opening a pull request.

[jgonera]

This is also a problem when using custom typeParsers or even when disabling them altogether with:

const pool = createPool('postgresql://', {
  typeParsers: [],
});

In this case timestamptz fields will be of type Date also not matching string | number. Given that type parsers can convert the value to absolutely any type I think QueryResultRowColumnType might need to be of type any. Unless I'm missing something there doesn't seem to be a way to assign proper static types to values that are dynamically converted.

It seems that this kind of mimics the Flow type in Slonik so I'll create an issue there too:
https://github.com/gajus/slonik/blob/e2ec2c7536fb20fcdc5dde8afaf4e3ebd0459c96/src/types.js#L181

[adevine]

I commented on the issue in Slonik that jgonera opened, but I agree, this type should really be any - having it be anything other than any gives an incorrect false sense of type safety. The default type parsers already parse JSONB fields as deserialized javascript objects, so column results can really be anything.

Also, from just a pure "ideology" perspective, my 2c is that it is better to leave query results essentially untyped. Right now I always do something like this:

const queryResult: any = await pool.query(sql` ... `);

While I could add a type to queryResult based on the column definition in my SQL, in my opinion that's a mistake because it's too easy to change the SQL string and not update the type definition. There is really no way to automatically know what the types are unless you take an approach like https://github.com/mmkal/slonik-tools/tree/master/packages/typegen and generate the types by watching the program as it executes (I think that @slonik/typegen package is super innovative but I'm not sure how I feel yet about making types be dependent on the runtime results).

[mmkal]

Agreed that string | number isn't right, and that we basically can't be specific because of type parsers. But I don't think any is the right choice either - it should be unknown. IMO now unknown exists any should only be used as a last resort. In this case the user can still cast to any as @adevine suggests, if they want.

Unfortunately changing to unknown would be a breaking change, so the best way to get it in might be by getting an equivalent change into slonik, presumably a major version bump. Then we can update the types here too.

[adevine]

I don't feel super strongly on this one, as I can always just cast to any as you point out, but my 2 cents is that using unknown would just make the developer experience worse while rarely providing any "real" type safety. The reason being, at the end of the day, there is no static way to validate that the SQL matches the result type you say it is (again, without doing something like @slonik/typegen does by inspecting the types at runtime) so you are either (a) trusting that your type definitions are manually in sync with your SQL or (b) forcing the user to do a ton of type assertions on anything that comes back from a query. Especially since there are a bunch of open issues that make narrowing unknown types particularly unwieldy (e.g. microsoft/TypeScript#21732).

I understand this is more of a philosophical issue, so again I could go either way, but in general I think that APIs that return copious amounts of unknowns, where those unknowns are always expected to be narrowed, result in a poor developer experience.

[mmkal]

@adevine that's a good point - it's possible that the type tooling could improve to mitigate unknowns in general, but for now this could be annoying. In the meatime, if we did use unknown, it'd still be possible to opt-out in a way that doesn't involve casting everywhere, by replacing the sql export from slonik with a version with a modified type:

import {sql as originalSql, TaggedTemplateLiteralInvocationType, createPool} from 'slonik'

export interface TypedSql<T> {
  (...params: Parameters<typeof originalSql>): TaggedTemplateLiteralInvocationType<T>
}

test('typed sql function', async () => {
  const sql: TypedSql<Record<string, any>> = originalSql

  const slonik = createPool('')
  const result = await slonik.one(sql`select * from foo`)
  result
})

Then the types work the way you're suggesting here. You could also replace Record<string, any> with something more specific:

This is basically a non-automated version of what @slonik/typegen does.

[orta]

Hi thread, we're moving DefinitelyTyped to use GitHub Discussions for conversations the @types modules in DefinitelyTyped.

To help with the transition, we're closing all issues which haven't had activity in the last 6 months, which includes this issue. If you think closing this issue is a mistake, please pop into the TypeScript Community Discord and mention the issue in the definitely-typed channel.