valtyr/prisma-kysely

Possibly incorrect generated types for DateTime fields

Closed this issue ยท 9 comments

First and foremost: thank you very much for taking the time to develop this tool ๐Ÿ™
Paired up with kysely, it makes an amazing setup for dealing with DBs in Node.

I have a prisma definition that contains this model:

model Match {
  // ...
  startedAt           DateTime             
  endedAt             DateTime?           
  createdAt           DateTime            @default(now())
  deletedAt           DateTime?         
  // ...
}

Which results in those types when using prisma-kysely:

export type Timestamp = ColumnType<Date, Date | string, Date | string>

export type Match = {
  startedAt: Timestamp
  endedAt: Timestamp | null
  createdAt: Generated<Timestamp>
  deletedAt: Timestamp | null
}

It feels right, but I stumble upon a problem when querying from this table. I use the following query

 const match = await kysely
    .selectFrom('match')
    .selectAll()
    .where('id', '==', id)
    .executeTakeFirst()

Which retrieves the following result:

{
  id: 5,
  startedAt: '2019-11-14T00:55:31.820Z',
  endedAt: null,
  createdAt: '2023-03-30 17:39:01',
  deletedAt: null,
}

But the generated type for the match variable is:

const match: {
    // ...
    startedAt: Date; // <-- this should be string
    endedAt: Date | null; // <-- this should be string | null
    createdAt: Date; // <-- this should be string
    deletedAt: Date | null; // <-- this should be string | null
} | undefined

Please correct me if I'm wrong. I don't know if it's prisma-kysely responsibility for parsing the dates inside prisma config file as string, or if it's kysely itself that should have been interpreting the Date field and parsing it as a string when retrieving data from the database.

Thank you very much for the attention and time

valtyr commented

Hey there. This is totally up to the database driver and Kysely dialect you're using. Some drivers return parsed JS dates, while others do not. The Postgres driver pg, for example, returns parsed dates. Which driver are you using?

As an escape hatch you can always override types in the generator config e.g.:

generator kysely {
  // ...
  dateTimeTypeOverride = "string"
}

Okay, so I suppose it is on Kysely to understand that my driver returns dates as strings?
My current config is the following:

import Database from 'better-sqlite3'
import { Kysely, SqliteDialect } from 'kysely'
import type { DB } from './types'

const database = new Database('./new-database.db')

export default new Kysely<DB>({
  dialect: new SqliteDialect({
    database,
  }),
})

And thanks for the datetimetype override tip, it solved my issue, but don't you think that kysely should be smart enough to infer the date types based on the driver?

@leo-diehl

don't you think that kysely should be smart enough to infer the date types based on the driver?

Kysely's API is dialect agnostic by design.
Kysely doesn't parse anything, underlying drivers/clients do. Some drivers/clients can be configured (e.g. pg-types) with custom parsing behavior. You could use/make a Kysely plugin that does additional transformations.

Any inference based on passed dialect would:

a. hurt maintainability/readability by adding lots of generics and complex types.
b. hurt compile performance and query complexity potential.
c. hurt ease of dialect creation/maintenance by 3rd parties.

@valtyr Thanks for this awesome project! ๐Ÿฅ‡

Having the ability to override select data types is very nice, but requires the consumer to investigate and know a bunch of stuff about the driver.

What if:

  1. prisma generation could be wrapped with a thin prisma-kysely CLI.
  2. consumer was allowed to provide an optional global hint to CLI - "I'm using sqlite" - to offer her a more precise output. e.g. prisma-kysely --sqlite.
  3. Kysely is postgres-first in many design decisions regarding defaults, so "hintless" runs would default to pg's behavior.

Alternatively, recommended data type overrides for each built-in Kysely dialect could be added to the documentation. No automation, but a nice gesture for sure.

Alternatively, the Kysely generator could be appended to consumer's file with a CLI, and a dialect hint would provide a more precise generator.

valtyr commented

TL;DR: if the types for SQLite are wrong we can update them in the next version.

Hey @igalklebanov. Thanks for some good ideas. We already have information about the database people are using from their prisma schemas and generate different types for each. Within each database type there are of course different drivers which could have inconsistent deserialization for different data types. I think the best we can do is support the most common driver for each database type. It could very well be that I've just picked the wrong DateTime type for sqlite. In that case I can update the type maps in the next version.

The override is only ever meant as an escape hatch, not a general solution by the way. I want 99% of users to be able to use the generator without using any overrides.

Also... the whole idea of this library is that people don't have to think about keeping their TS types up to date. They should just be able to use the prisma migrate/generate cli. Otherwise they could just use kysely-codegen (which is nice of course but isn't as tightly bound to the migration flow and has in my case often led to inconsistencies between the database schema and the TS types). Therefore I think that all configuration should happen through the Prisma schema rather than going through a separate command line interface.

@valtyr Thank you for the detailed comment. ๐Ÿ™

I mixed database and driver and generally "brain farted". ๐Ÿ˜…

Sounds good to me, and I totally agree with the direction.

I think a good starting point is to support the underlying drivers the built-in Kysely dialects use (better-sqlite3, pg and mysql2).

valtyr commented

@igalklebanov that makes perfect sense. I'll go through and double check that I have the correct types for the standard drivers. Thanks for the input! ๐Ÿ˜Ž

valtyr commented

Alright! @leo-diehl I have a snapshot version for you to try: 0.0.0-snapshot-20230403105456. If you could remove the override and confirm that this version is working for you I'll release a version with the changes.

image

@valtyr it seems to be working as expected!

valtyr commented

Awesome. Thanks for catching this issue @leo-diehl!