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

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

Open
inorganik opened this issue Dec 21, 2023 · 6 comments · May be fixed by #750
Open

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

inorganik opened this issue Dec 21, 2023 · 6 comments · May be fixed by #750

Comments

@inorganik
Copy link

inorganik commented Dec 21, 2023

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):

  • OS: macOS
  • Browser: Chrome 120
  • Version of CLI: 1.93.0
  • Version of supabase-js: 2.38.0
  • Version of Node.js: 20.5.1
@inorganik inorganik changed the title Generate types: "Json" type is incompatible with valid JSON objects Generate types: "Json" type is incompatible with other types Dec 21, 2023
@sweatybridge
Copy link
Contributor

sweatybridge commented Dec 22, 2023

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.

@sweatybridge sweatybridge transferred this issue from supabase/cli Dec 22, 2023
@inorganik
Copy link
Author

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

@isaacharrisholt
Copy link
Contributor

@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
Copy link
Author

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
Copy link
Contributor

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
Copy link

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

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 a pull request may close this issue.

4 participants