SweetIQ/schemats

Change json/jsonb type

abenhamdine opened this issue ยท 6 comments

Currently, a json or jsonb db field is mapped with typescript Object type.
Though Object would seems appropriate at first sight, it leads to following type errors :

Property 'foo' does not exist on type 'Object'.

...sinceObject type (same with object and {} ts types) doesn't accept more properties.

It should be more appropriate to type these fields with a specific type which accepts any properties, something like :

interface JsonParsed extends Object {
    [k: string]: any
}

or more precise :

interface JsonParsed {
        [k: string]: string|number|boolean|Date|JsonParsed|JsonParsedArray|null;
    }
interface JsonParsedArray extends Array<string|number|boolean|Date|JsonParsed|JsonParsedArray|null> { }

What do you think about it ?

xiamx commented

Looks good!

In our use-case, we have specific interface for each jsonb object, and we extend the generated schemats table interfaces with our own. i.e.

interface SomeJsonbType {
  foo: string
  bar: number
}

import { ATable } from './dbname.schemats'

export interface OurTable extends ATable {
  somejsonb: SomeJsonbType
}

I think most users wouldn't probably want to go through the same level of verbosity, so having a more generic jsonb type is appropriate.

My only concern with choice number 2 is about whether each driver applies their own parsing method to json and jsonb -- i.e. Date is not a part of json value but drivers may decide to parse ISO datestring into Date objects.

I'm leaning more towards

interface JsonParsed extends Object {
    [k: string]: any
}

My only concern with choice number 2 is about whether each driver applies their own parsing method to json and jsonb -- i.e. Date is not a part of json value but drivers may decide to parse ISO datestring into Date objects.

Good remark.
Though the more I think about it, the more the 2nd choice looks better, because any actually disable typechecking, so nothing would prevent to do something like :

interface Json extends Object {
            [k: string]: any
        }
const json: Json = { foo: 'bar' }
json.myMethodWhichDoesntExist('foo') // NO TS ERROR

while with 2d implementation :

interface JsonParsed {
            [k: string]: string | number | boolean | Date | JsonParsed | JsonParsedArray;
        }
        interface JsonParsedArray extends Array<string | number | boolean | Date | JsonParsed | JsonParsedArray> { }

const jsonParsed: JsonParsed = { foo: 'bar' }
jsonParsed.myMethodWhichDoesntExist('foo') // TS ERROR

You're right, json parsing result may vary depending on the driver (and for instance, for node-postgres, there's still a debate on how dates should be parsed as Date object or no, see brianc/node-pg-types#50).

Besides, some users may choose to override the types parsed by the driver in their application code (as you know, node-postgres allow this, I don't know for other drivers).

I think it's an acceptable tradeoff. And in a second time, it will be still possible to implement an option to generate the JsonParsed type with or without the Date type in the union.

xiamx commented

Sounds good, let's go with

interface JsonParsed {
    [k: string]: string | number | boolean | Date | JsonParsed | JsonParsedArray;
}
interface JsonParsedArray extends Array<string | number | boolean | Date | JsonParsed | JsonParsedArray> { }

const jsonParsed: JsonParsed = { foo: 'bar' }
jsonParsed.myMethodWhichDoesntExist('foo') // TS ERROR

Should be noted, though, that postgres allows any json value to be typed as json, and does not limit it to objects or arrays:

> SELECT '"foo"'::json;
json
----
"foo"
ianp commented

Really JSON and JSONB should be typed as unknown because you should really be running them through type guards before doing anything with the contents.