Skip to content

Better sql`` types #84

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 12, 2021
Merged

Conversation

Minigugus
Copy link
Contributor

  1. Set row type to YourType | undefined in sql<YourType>``; but to YourType in sql<YourType[]>``; so that the user can choose the best regarding the situation (see also Typescript support #8 (comment)) :
const [user] = await sql<User>`SELECT * FROM users WHERE id=${id}`;
user.name // error : `user` can be `undefined`
user?.name // ok

for (const user of await sql<User[]>`SELECT * FROM users WHERE id=${id}`)
  user.name // ok since `user` will always be defined

await sql<User>`SELECT * FROM users WHERE id=${id}`.cursor(user => {
  user.name // ok (same as `for`), supported by both the `User` and the `User[]` form
});

const [user] = await sql<[boolean]>`SELECT * FROM users WHERE id=${id}`;
user.valueOf() // incorrect but ok (the tuple form accept any type)
  1. Currently, primitive types are not permitted as row types, even in sql<number>`SELECT 1`;. This PR add T to { '?column?': T } conversion if T is Serializable (string, number, Date, ...) :
const [{ '?column?': value }] = await sql<number>`SELECT ${nb}`; // ok
// => value: number

Feel free to discuss types here before the release 👍

@yckao
Copy link

yckao commented Jun 20, 2020

Looks good.
But maybe add some example and docs into README will be better since the difference between sql<User> and sql<User[]> maybe not very intuitive.

@yckao
Copy link

yckao commented Jun 20, 2020

In my opinion, maybe only set row type to YourType | undefined when using tuple sql<[YourType]> will be more intuitive.
(Just because we could write something like const [user] = await sql<[User]>`SELECT * FROM users WHERE id=${id}`;, and [user] is like [User] 😆)
And if we can use sql<[YourType]> and sql<YourType[]>, maybe remove sql<YourType> will be less confusion.

@Minigugus
Copy link
Contributor Author

Minigugus commented Jun 20, 2020

(Just because we could write something like const [user] = await sql<[User]>`SELECT * FROM users WHERE id=${id}`;, and [user] is like [User] 😆)

I agree but currently it's not possible in TypeScript to transform types in tuples without loosing types positions (i.e. [A, B] => [A | undefined, B | undefined] not possible, only => (A | B | undefined)[]). Also, in my opinion, when we pass [User] I think we expect to get [User] as a result, not [User | undefined]. I wanted to keep tuples for complex use cases not compatible with other forms.

But in the meantime I found another possible solution 😆 The problem is that we loose tuples 😢

@yckao
Copy link

yckao commented Jun 20, 2020

I agree but currently it's not possible in TypeScript to transform types in tuples without loosing types positions (i.e. [A, B] => [A | undefined, B | undefined] not possible, only => (A | B | undefined)[]).

Partial will work with tuple.
Playground Link

And I think tuple usage is more fit in this situation, because most situation we need to handle undefined like this is when we using destructuring with known length of elements, and is same to the usage of tuple.
Is there any other complex usage you mention above I missed?

Also just wondering if const [user] = await sql<[User | undefined]>`SELECT * FROM users WHERE id=${id}`; will work with new typing?

@Minigugus
Copy link
Contributor Author

Partial will work with tuple

Oh nice that's what I was looking for 👍

And I think tuple usage is more fit in this situation, because most situation we need to handle undefined like this is when we using destructuring with known length of elements, and is same to the usage of tuple.
Is there any other complex usage you mention above I missed?

What about const [{ n }] = sql<[{ n: 42 }]>`SELECT 42`;? Here n will always be 42, not 42 | undefined. My opinion is that we should let the user decide where a row is defined or not.

Also just wondering if const [user] = await sql<[User | undefined]>`SELECT * FROM users WHERE id=${id}`; will work with new typing?

It should as tuples are passed as is. 😉

@yckao
Copy link

yckao commented Jun 20, 2020

What about const [{ n }] = sql<[{ n: 42 }]>`SELECT 42`;? Here n will always be 42, not 42 | undefined. My opinion is that we should let the user decide where a row is defined or not.

That seems reasonable.

And next problem is if the usage of sql<User> is reasonable if we can simply achieve the usage with
const [user] = await sql<[User | undefined]>`SELECT * FROM users WHERE id=${id}`;
and
const [user] = await sql<[User?]>`SELECT * FROM users WHERE id=${id}`;
and
const [user] = await sql<Partial<[User]>>`SELECT * FROM users WHERE id=${id}`;
and
const [user] = await sql<Partial<User[]>>`SELECT * FROM users WHERE id=${id}`;

I think sql<User> and sql<User[]> is really confusing.

And I also notice that the type of
const [user] = await sql<[User | undefined]>`SELECT * FROM users WHERE id=${id}`;
is currently User not User | undefined even in this commit.

Never mind, I was test it outside tsconfig 😢

@Minigugus
Copy link
Contributor Author

Minigugus commented Jun 22, 2020

@yckao So you suggest to drop support for sql<User> with a contraint like T extends any[]? I think not being able to pass sql<User> is confusing (and I cannot reject arrays as they are infered as objets), but maybe I'm wrong.

What about a survey 😆? React to this message with:

  • 👍 for sql<User[]> only
  • 👎 for sql<User> only
  • 😕 for both
  • (none) for anything else (please comment 😉)

@Minigugus
Copy link
Contributor Author

Minigugus commented Jun 23, 2020

Ok finally I think you're right: sql<User[]> is better for users but also for TypeScript (infer better):

const [maybeUser]: [User?] = await sql`SELECT * FROM users WHERE id = ${id}`; // ok
// => maybeUser: User | undefined

// @ts-expect-error
const [invalid] = await sql<42>`SELECT 42`; // fails: `42 not assignable to any[]`
const [valid]: [undefined] = await sql`SELECT null`; // ok but useless
// => valid: undefined

But undefined is still not enforced in case the query cannot returns 0 rows, or if the user check array bounds correctly:

// Tuple mapping now works with the `any[]` constraint on `T`:
// `[42]` mapped to `[{ '?column?': 42 }]`
const [{ "?column?": n }] = await sql<[42]>`SELECT 42`; // ok
// => n: 42

const result = await sql<User[]>`SELECT * FROM users WHERE id = ${id}`; // ok
if (!result.length)
  throw new Error('Not found');
return result[0]; // => result[0]: User

const result = await sql<User[]>`SELECT * FROM users WHERE name LIKE ${pattern}`; // ok
return result.map(user => user.id);

To conclude, I think the following form is peferable when destructuring:
const [user, maybeUser]: [User, User?] = await sql`...`; // ok
// => user: User
// => maybeUser: User | undefined

const [first, second, third]: [User, User?, User] = await sql`...`; // fails
//                                        ^^^^^^ 
//           A required element cannot follow an optional element

const [first, second, third]: [User, ...(User | undefined)[]] = await sql`...`; // ok
// => first: User
// => second: User | undefined
// => third: User | undefined

For any other case, sql<YourType[]>`...`; works fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants