mmkal / pgkit

PostgreSQL🤝TypeScript monorepo. SQL client/admin UI/smart migrator/type generator/schema inspector
https://pgkit.dev
190 stars 25 forks source link

@slonik/typegen – Discussion – Upcoming feature: User Type Enhancements #386

Closed janpaepke closed 2 years ago

janpaepke commented 2 years ago

the problem

We can't rely on typegen to always be 100% perfect, for example when it can't determine nullability from complex queries or with json columns. In these cases we want to be able to "help it out" and modify the generated type output.

We can't modify the generated types, as generated code shouldn't be touched. It would be overwritten in subsequent code-gen-runs.

We currently also can't modify the type assignment in the template literal, as it also gets overwritten on every run.

the planned solution

Instead of overwriting the whole type definition for a processed template literal, we'll preserve modifications like intersections.

Example:

sql`select json_b_col from table`

after generation will become

sql<queries.TestTable>`select json_col from table`

export declare namespace queries {
  // Generated by @slonik/typegen

  /** - query: `select json_col from table` */
  export interface TestTable {
    /** column: `example_test.table.json_col`, regtype: `jsonb` */
    json_col: unkown
  }
}

this can then be manually modified to

interface QueryEnhancement {
    json_col: { content: string }
}

sql<queries.TestTable & QueryEnhancement>`select json_col from table`

export declare namespace queries {
[...]
}

The additional type intersection with QueryEnhancement will be left untouched on subsequent runs.

In short: We can increase specificity with intersection types. This method is very explicit, easy to read and keeps the type definition where it belongs.

up for debate

There are two things I have a strong opinion on, but would like some feedback from other users:

  1. Allow other type modifications like completely overwriting extracted types i.e. Omit<queries.TestTable, 'col'>

I don't think we should allow this, because this has the potential for silent errors. If a col type changes but is overwritten in place, the dev will not be alerted to this new query result type. It will pretend it's still the same (overwritten) type. What we want to do is increase specificity if slonik/typegen is not sure enough, not completely change the returned type. In case a dev wants to use types differing from the typegen's result, he should make slonik/typegen ignore the query. At least that's explicit.

  1. Overwrite on initial run

When typegen runs for the first time on a query, which already has typedefs in there, i.e. sql<{col: string}> I would keep the current behaviour and completely overwrite it to sql<queries.TestTable>`...`, rather than keeping it and resulting to smth. like sql<queries.TestTable & {col: string}>`...`

The rationale here is that if you are running slonik/typegen on a query for the first time which has typedefs, then the reason for those defs to be there is probably because you previously didn't have slonik/typegen. So keeping them doesn't make much sense.

Looking forward to your feedback!

mmkal commented 2 years ago

Closed by #389