Skip to content
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

change(typegen: TS): Export type, not interface #689

Merged

Conversation

isaacharrisholt
Copy link
Contributor

What kind of change does this PR introduce?

Minor change

What is the current behavior?

Currently, the TypeScript type generation exports the Database type as an interface. However, this is somewhat problematic in that TypeScript interfaces don't implicitly define an index signature for their keys.

What do I mean by this?

Consider the following example:

type Pokemon = {
  name: string
  type: string | [string, string]
}

type Pokedex = Record<string, Pokemon>

interface MyPokedex {  // Note: interface!
  bulbasaur: Pokemon
  charmander: Pokemon
  squirtle: Pokemon
}

function getPokemon<T extends Pokedex>(pokedex: T): (keyof T)[] {
  return Object.keys(pokedex)
}

I would expect to be able to call getPokemon with MyPokedex as the type parameter. However, this results in a type error:

Type 'MyPokedex' does not satisfy the constraint 'Pokedex'.
Index signature for type 'string' is missing in type 'MyPokedex'.

What's the problem?

This means that if a library author wants to create a generic function where the expected type parameter is the user's Database type, there's no way for them to create a constraint.

For example, isaacharrisholt/supawright uses the user's Database to accurately type input and output types for some of its functions (much like supabase-js). It uses the following code to constrain the type parameter passed into the withSupawright function:

import type { GenericSchema } from '@supabase/supabase-js/dist/module/lib/types'

export type GenericDatabase = Record<string, GenericSchema>

(yes, I'm stealing from Supabase, but it works)

However, if I try to have withSupawright<Database extends GenericDatabase>() and then call it with the exported Database interface, I get the error mentioned above:

Type 'Database' does not satisfy the constraint 'GenericDatabase'.
Index signature for type 'string' is missing in type 'Database'.

Changing the exported interface to instead be a type resolves this.

We've been running the following...

types:
    pnpm supabase gen types typescript --local | \
    sed 's/export interface Database {/export type Database = {/' \
    > src/types/database.ts

...in production for a while now, and it has no negative impact on the Supabase client, and doing SupabaseClient<Database> still works as expected.

It would make Supabase extension libraries like this one much easier to write, and I can't think of any downsides to this change.

I might be missing something huge, in which case, let me know.

What is the new behavior?

The TypeScript type generator returns a type, not an interface.

@isaacharrisholt isaacharrisholt requested review from a team as code owners January 11, 2024 21:27
@soedirgo
Copy link
Member

Thanks @isaacharrisholt! There's little reason to use interface now that I think about it, so unless this breaks existing projects this change LGTM.

@kamilogorek
Copy link
Member

It can break if:

  • someone extends a database with interface ExtendedDatabase extends Database {}
  • someone uses interface merging interface Database { custom_schema: {} }

So in theory it is a breaking change, but at the same time, it's:
a) generated code
b) very low change of happening

@soedirgo
Copy link
Member

In this case I think it's worth the risk - we can reconsider if we get complaints about it.

@soedirgo soedirgo force-pushed the change/typescript_type_export branch from 7ef71b0 to 5730861 Compare January 31, 2024 08:48
@soedirgo soedirgo merged commit 077a60d into supabase:master Jan 31, 2024
4 checks passed
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