-
-
Notifications
You must be signed in to change notification settings - Fork 582
Add JSON type #102
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
Add JSON type #102
Conversation
The difference here is that arbitrary JSON can be used as inputs and outputs instead of a string, correct? Why is this desirable to you? I decided to go with a string for JSON because allowing arbitrary objects kinda "breaks" the purity of GraphQL if you will. Of course I'm willing to change my mind on this one ofc, but I'm curious to hear why you want this change. A couple of other questions, why doesn't |
I started with strings on my end as well. What eventually made me switch was that managing manually deserializing things on the Relay end was just annoying. @leebyron said this was okay on Twitter a while ago. |
I personally just don't like the idea of doing I tried creating custom types to represent the data but PostgraphQL (or, more likely, the create type t_something as (
qux integer,
wibble varchar
);
create type t_whatever as (
foo integer,
bar varchar,
baz t_something
);
create function my_table_computed_column(o_table my_table) returns setof t_whatever as $$
select cast(row(...) as t_whatever) from some_table;
$$ language sql stable; fragment on MyTable {
rowId
computedColumn
} It'd return something like (again: from memory): {
"rowId": 7,
"computedColumn": [
"{8,foo,...}",
"{79,bar,...}",
]
} i.e. it would detect the array-ness but after that it was just one stringified version of the composite type. Situations like this crop up and having easily accessible JSON as a temporary or even longer term workaround is quite handy. |
Yep, that looks right. Composite object types are definitely on my radar to support. I think that's the correct solution as it's type safe. I'm working to build in support (unfortunately life stuff keeps happening so it's going slowly, but it's going to come with some other awesome things 👍). The If a JSON object can be a scalar type this breaks some guarantees would could make about GraphQL. Without a schema and the parsed query (in case of aliases) there's no way to know where in the JSON tree the leaf node begins. This could potentially make GraphQL clients harder to build especially in a data normalization phase where objects are normalized by I'd love to hear @leebyron and someone from the Apollo team's perspective here (I believe @helfer is a good cc). |
I haven't had any issues with that so far. Generally we're using JSON in GraphQL for JSONB columns in Postgres. |
I don't see any issues with having it in the GraphQL spec either. Apollo Client doesn't use the schema at the moment. The JSON objects would get normalized as well, but that shouldn't cause any problems, because the id mapping is determined by the |
Here's the Twitter thread: https://twitter.com/jimmy_jia/status/725715287115456512 |
I'm still not super comfortable with this, but I'm willing to compromise 😊 So this would be a breaking change, and a feature I'm not yet comfortable making the default so let's put it behind a feature flag. There's currently no good way to do flags (will be fixed in the next version) so for now just set some global variable in Also, can we copy the implementation instead of adding another dependency? It should be a simple enough implementation. Finally, tests. Thanks 👍 |
You're probably going to be better off pulling in the dependency. It's explicitly set up as a minimal dependency for optimal reuse, and I do plan on updating it once things settle down with proper null support in literals. |
As an update, I’ve fixed JSON serialization here #121. It was always supposed to use |
Thanks to @taion for implementing graphql-type-json; I've just added it in to postgraphql - seems to work well for my purposes.