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 json/jsonb type #80

Open
abenhamdine opened this issue Jan 14, 2018 · 6 comments
Open

Change json/jsonb type #80

abenhamdine opened this issue Jan 14, 2018 · 6 comments

Comments

@abenhamdine
Copy link
Contributor

abenhamdine commented Jan 14, 2018

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

xiamx commented Jan 14, 2018

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
}

@abenhamdine
Copy link
Contributor Author

abenhamdine commented Jan 15, 2018

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

xiamx commented Jan 15, 2018

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

@xiamx
Copy link
Contributor

xiamx commented Jan 15, 2018

@crispmark

@anup-the-magic
Copy link

anup-the-magic commented Mar 19, 2019

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

ianp commented Sep 24, 2019

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.

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

No branches or pull requests

4 participants