kysely-org / kysely

A type-safe typescript SQL query builder
https://kysely.dev
MIT License
10.74k stars 271 forks source link

Add documentation regarding best way to specify a nullable column #27

Closed martinblostein closed 2 years ago

martinblostein commented 2 years ago

I see two sensible ways to specify type corresponding to a nullable SQL column:

interface Entity {
  attribute: string | null;
}
interface Entity {
  attribute?: string;
}

It's not clear in the documentation I was able to find which should be preferred when using kysely. In practice I see an advantage for each kind:

I'm not clear if there are other consequences for choosing one over the other. If either is fine, I think the pros/cons should be noted in the documentation. If one if preferred that should definitely be noted! :)

(The two options could be combined, or a union with undefined could be thrown in the mix to result in string | null | undefined (yuck), but I think it's obvious that the user ought to avoid that and so I want to focus on these two options for now.)

koskimas commented 2 years ago

string | null is the correct type since the value can either be a string or a null. I don't know how to document this without stating the obvious. I could add nullable values to some examples maybe?

I tried to implement the insert type so that you could leave out nullable values, but wasn't able to make that work. I'll give it another try.

martinblostein commented 2 years ago

string | null is the correct type since the value can either be a string or a null. I don't know how to document this without stating the obvious. I could add nullable values to some examples maybe?

If you recommend that users use string | null instead of an optional field to represent a nullable column, you can just say so. For example, the doc could read:

To specify a nullable column of type T, use T | null. This maintains an intuitive mapping between SQL NULL and Typescript null. Do not use an optional property.

I don't see the downside to stating this when javascript provides at least three ways to represent a missing value (null, property set equal to undefined and property not present). NULL <=> null may seem 'obvious' but it's still a mapping between different systems.

I tried to implement the insert type so that you could leave out nullable values, but wasn't able to make that work. I'll give it another try.

Well, you can use an optional property instead! ;) I'm really not that concerned about the convenience factor of leaving out the null values on insert, I'll use happily T | null.

koskimas commented 2 years ago

Well, you can use an optional property instead! :)

No, that's the wrong type. null is null and undefined is undefined. Those are not different ways to represent the same thing.

For example, if you define a property as optional using ? or as string | undefined and you get a null value from the db, this code would be incorrect

if (person.firstName !== undefined) {
  // Here the type would be narrowed down to `string` from `string | undefined` but
  // the value could still be `null` leading to errors.
  person.firstName.toLowerCase() // This would throw, but typescript would be happy
}

So if the value can be null the only correct way to type it is to give it a type T | null .

martinblostein commented 2 years ago

Ah, of course, the DB engine will return null in either case, duh! So using ? will break the types. In this case I think it's even more important to say in the docs:

To specify a nullable column of type T, use T | null. Do not use an optional property to represent a nullable column, because the resulting types will be incorrect when using selectFrom!

Yes, it's obvious, but I think it's important to note. Lots of fools besides me will make the same mistake!

koskimas commented 2 years ago

Where do you think I should say that? In the readme somewhere?

koskimas commented 2 years ago

The example in the readme now has this

interface Person {
  id: number
  first_name: string
  // If the column is nullable in the db, make its type nullable.
  // Don't use optional properties.
  last_name: string | null
  gender: 'male' | 'female' | 'other'
}
martinblostein commented 2 years ago

Great, I was just going to suggest the same thing -- adding an example with a comment! Cheers.

samuelstroschein commented 2 months ago

EDIT: the Selectable<Table> types cast null to undefined | null | T. Sick type wrestling! Aka everything works.

@koskimas string | null leads to breaking changes in the types. see https://github.com/opral/lix-sdk/issues/55#issuecomment-2336897655.

Do you have an idea how to use the kysely schema Selectable etc. without running into the breaking change behavior described in the linked issue?

Using T | null | undefined would likely be confusing.