supabase / postgres-meta

A RESTful API for managing your Postgres. Fetch tables, add roles, and run queries
https://supabase.com
Apache License 2.0
941 stars 126 forks source link

Generate types: "Json" type is incompatible with other types #676

Open inorganik opened 11 months ago

inorganik commented 11 months ago

Describe the bug When using the generate types feature for your Database (which is mostly great), if your column type is json or jsonb, the generated type it gives you is this:

export type Json = string | number | boolean | null | { [key: string]: Json | undefined } | Json[]

This type is incompatible with any type you try to use it for. For example, if I'm using a column to hold an ingredient (this is hypothetical):

export interface Ingredient {
  id: number
  name: string
  quantity: string
}
const result: Json = { id: 1, name: 'Berries', quantity: '1 cup' } // this is how the object will be typed using the generated types
const ingredient: Ingredient = result // Type '{ [key: string]: Json | undefined; }' is missing the following properties from type 'Ingredient': id, name, quantity(2739)
const ingredient2: Ingredient = result as Ingredient 
// above results in:
// Conversion of type '{ [key: string]: Json | undefined; }' to type 'Ingredient' may be a mistake because neither type sufficiently 
// overlaps with the other. If this was intentional, convert the expression to 'unknown' first.

I have more examples of this in the typescript playground below. Given how error-prone it is to add a type for JSON data, can you please consider using any for Json types? We have to cast these values as any anyway because you cannot cast the type you are using, as demonstrated above.

To Reproduce You can see this behavior on this TypeScript playground link:

https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBDAnmYcBSBnCA7OBeODGKAS2wHM4AfObAVwFsAjYKauJiCAG2AENcNet27sA3nADaAa2CIAXIWJlyAXUWYc7OtgAmwAGZlguuAF92m7JNUAoewHoHcAKIANAIIBZAAoAZFzgARjgAWjgIJgArYABjGAx7UEhYODIYVgM+WNQASQooExJgbHgxWzg03UV6ZlYK2j4GYEUiUgoGgEc6ARgSJFblDrNbZOh4dMzs1AAlOJIUOHLK7CaWpXbyAG4GlULdYtKMRXzyfcOYGx2R21icIjhCjDpuGAAGDSxcAgkSauCADSNZqKADkACFWKRgBhQUDur1+go4KCQrE6GBQeZbvcJgUiiUYCd8QdCfhHjCXu84E44AAVZCoUESGRyQabdToL7aPSGYy6LbmLEkDBwBgijAqBAAC1QBh43AgAHcpWAoBAULBiqKDOqGAhGSjTudCaDFH8gatmvCeqUkQAKABMAHYAMwATgAlDjsA89gTSo7iWcA-ACE8qW84HxRcbQ3BbLS+JwAG6oCOvUVkeSJ5wAYRwaagkq0EAMBsWzKksmRbRUnKsPP0RmwJkFZixMAgFaZcdJpSxDD4iA4qD4YpFMD4slHsT4dAwqFb-VlbCQi2eBiMsQu3BHuYiRe4fDAopVMGlMtQEAvrAAdHBcuWLyK4EqY2lSoSSDg+NwgXc2BFvAt5wMkTwlrgXYojo0jYMq2BYkYxYwHe9iAQ8GZElyWg-A0KxrGCADKDBcC+wBwvhn4hv2CSKJIVGVJUvz-EElqESikJQNCsI2oiAwomiGJYmYAKMUxLGKGxwLrKCXgkNw0hwnACJ2gJoKOnA6KniJYlMfpSxVFJ7EgiiAASOByMpql9OpIQwEwGCYuYelMXYNwYfAhQ7igihzD5qDhpSryCrSDKViyNbsvWnxaDQOjNvy7bCqK4oYJKFBXnA8oiMqqrqpqfQwtleo9ii-kLBRNRrEC-q0RgTpul6jjOO43j+IEmnhHw3HDuhuIUs8rxBjh3xSFRkmAjJYJcTx1m2rZyKolpwkuRNRlTVasnyYp838UtmnabCa36ZN0lbWCFmtoge1qUt9mOc5om2HYPp+iSFzBiapTklhjqSG8qg0s44VMlYKW0De0bpSQ5CrEwvAIN2669h9ppoQebW+AEcCumEGxStw-SsH+-W+l5wUwK6sVjaC8oQKC9go3AABiXDknTXCM298CgE0YC8IobPdkFQ1U8D9KGqCdYUBD8HwDGkpw8miPQczoIAET0xroIY7YQA

Desktop (please complete the following information):

sweatybridge commented 11 months ago

We probably want to type this as generics here, ie.

export type Json<T=void> = string | number | boolean | null | T | T[]

I will transfer this to postgres-meta repo as it's a feature for the types generator.

inorganik commented 11 months ago

Thanks, can you please link to where you moved it?

isaacharrisholt commented 10 months ago

@sweatybridge from my experimentation, using generics wouldn't make any difference, since the type in the exported interface would need to either be Json using the default type parameter, or would need to be instantiated with some fixed type parameter.

x: Json
y: Json<SomeFixedTypeParam>

Alternatively we make the Database type a generic type, but that's not suitable, and would force all Json types within to take on the same type anyway.


@inorganik the problem with using something like any or even doing something like:

export type Json<T = Record<string, any>> = string | number | boolean | null | T | Json<T>[]

is that it would likely break many people's builds, as a lot of projects will have the no-explicit-any rule enabled in eslint.

We could add an ignore at the top of the file, but that doesn't solve the problem for anyone - some people use other linters.

People could manually add the ignore by piping through another command, but it's not really fair to expect people to do that.

Appreciate that I'm providing problems not solutions here, but I also feel like doing:

const ingredient = result as unknown as Ingredient

Isn't too egregious, and is fairly common anyway.


Actually, I came up with a (potential) solution!

Since you're likely to need to use as anyway to type things coming from Json, we could just change the Json type to be the following:

export type Json = string | number | boolean | null | unknown | Json[]

const result: Json = { id: 1, name: 'Berries', quantity: '1 cup' }
const i: Ingredient = result as Ingredient

This then throws no errors.

However, this identical to doing export type Json = unknown, so we could just remove Json entirely, if we were to go down this route. Interested to hear what people's opinions are.

inorganik commented 10 months ago

I agree, I think it would be more productive to use unknown and remove the Json type entirely. Then we can cast freely and not violate any ESLint rules.

isaacharrisholt commented 10 months ago

I agree, I think it would be more productive to use unknown and remove the Json type entirely. Then we can cast freely and not violate any ESLint rules.

I think the first step would be to change the export to export type Json = unknown , but mark it as deprecated, and perhaps include a warn message in the Supabase CLI.

vyas-meet commented 5 months ago

This would be great. Seems like a simple change to move forward with unknown. Hope it's implemented soon.