Improve typing - Generic type should accept an object instead of an object[]

[Newbie012]

Currently, in order to type a query, we need to pass it as generic as an array:

const q = await sql<{ text_col: string }[]>`select text_col from table_name`;

While it doesn't introduce any bugs, it's redundant to manually type it as an array (type[]) since it will always return an array.

I maintain a library (SafeQL) that generates types based on queries (which uses this library, thanks to .describe()). While writing compatibility guides for each SQL library, I found out that this is the only library that requires adding [] at the end of the type.

Since it's a breaking change, I will fully understand if you would prefer not to merge it.

Although, there's a possibility to both support singles and arrays while maintaining backward compatibility, but it over complexes the type (see demo):

<T extends readonly (object | undefined)[] | (object | undefined) = Row>(template: TemplateStringsArray, ...parameters: readonly (SerializableParameter<TTypes[keyof TTypes]> | PendingQuery<unknown>)[]): PendingQuery<T extends ReadonlyArray<any> ? T : T[]>;

Note the new T:

- T extends readonly (object | undefined)[] = Row[]
+ T extends readonly (object | undefined)[] | (object | undefined) = Row[]

and the new return type:

- PendingQuery<T[]>;
+ PendingQuery<T extends ReadonlyArray<unknown> ? T : T[]>;

Results in:

On another note - There's a drift between the declaration types of nodejs and deno. I'm not sure if this is intentional.

[porsager]

I have a v4 coming up with other breaking changes, so if @Minigugus thinks this is a good idea, i have no objections.

[Minigugus]

@Newbie012 Initially, sql<RowType> was supported but was removed to only support sql<RowType[]> (see https://github.com/porsager/postgres/pull/84#issuecomment-647465191). The idea was to allows tuples and to kind of support multi-statement queries with sql.unsafe:

const [user, bug] = await sql<[User?]>`SELECT * FROM users WHERE email = ${email} LIMIT 1`;
// => error because `[User?]` only have 1 element but 2 requested

const [
  someTableRows,
  anotherTableRows
] = await sql<[SomeTableRow[], AnotherTableRow[]]>`
  SELECT * FROM someTable;
  SELECT * FROM anotherTable;
`;

(I haven't tested the unsafe example, @porsager is this still valid (multi-statements adds 1 level to the returned array)?)

Maybe it's time to reopen the debate, I guess it's up to the community now

[Newbie012]

If I understand correctly:

1. sql supports multi stmts.
2. When there's a single stmt, the result should be T[].
3. When there are two stmts, the result should be [T[], R[]].
4. When there are three stmts, the results should by [T[], R[], X[]], etc...

Then I assume this could work out?

[porsager]

(I haven't tested the unsafe example, @porsager is this still valid (multi-statements adds 1 level to the returned array)?)

unsafe queries without any arguments are implicitly sent as simple queries allowing for multiple statements. This is also possible with the next release using tagged template literals by calling simple like this: sql`select 1;select 2`.simple() . I am considering changing unsafe in v4 to require using simple().

[porsager]

@karlhorky could you maybe help figure out if this is something that should be merged?

[Newbie012]

While I still think my suggested approach makes a bit more sense, I would advise not going for it since the added value here is not worth the breaking change.

Feel free to close this PR 🙂

[karlhorky]

I think the main value is this, from the original PR description:

While writing compatibility guides for each SQL library, I found out that this is the only library that requires adding [] at the end of the type.

If consistency with other libraries in the ecosystem is important for Postgres.js, then maybe the approach of accepting an object instead of an array or tuple would be better as the only API for generic arguments (as a breaking change for v4).

One other downside not discussed explicitly above is the other benefits that can come from using tuples as the generic arguments data type, as @yckao and @Minigugus mentioned in various #84 comments. In our team we switched away from using tuples because SafeQL does not support the tuple notation and TypeScript can be made to check for indexed access with noUncheckedIndexedAccess. So potentially tuples are less useful because of this.

So I would suggest one of two options:

1. If consistency with other library generic arguments is important to Postgres.js, then switch to object-only (remove array and tuple support) in v4
2. If it's not very important, or not important now, then close the PR

[porsager]

Thanks to both of you for the follow up and explanations.

How about the 3rd option?

Although, there's a possibility to both support singles and arrays while maintaining backward compatibility, but it over complexes the type (see demo):

<T extends readonly (object | undefined)[] | (object | undefined) = Row>(template: TemplateStringsArray, ...parameters: readonly (SerializableParameter<TTypes[keyof TTypes]> | PendingQuery<unknown>)[]): PendingQuery<T extends ReadonlyArray<any> ? T : T[]>;

What are the downsides of this?

[porsager]

About typescript decisions in this project, i rely on @karlhorky and @Minigugus to guide it, if it doesn't affect the js API 😉

[karlhorky]

If I'm understanding correctly, the third option above is to support both arrays and objects?

I guess I would suggest avoiding this, seems like a bigger downside (more complexity in implementing and maintaining it) without too much upside (allowing object generic arguments seems mainly good for compatibility, and I guess also supporting arrays would be for backwards compatibility). The combined benefits of library compatibility and backwards compatibility don't seem to outweigh the downside of the increased complexity - for me at least.

So I would probably still stay with option 1 or 2 - could go either way on which one, I think they have close to equal upsides and downsides.

Maybe @Minigugus has a different opinion.

[porsager]

Ok :) Thanks a lot... Let's close this then, and perhaps revisit later if @Minigugus thinks it's worth considering. (haven't heard from him in a while).

Thanks for the effort @Newbie012