8000 Add JSON type by benjie · Pull Request #102 · graphile/crystal · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Closed
wants to merge 1 commit into from
Closed

Add JSON type #102

wants to merge 1 commit into from

Conversation

benjie
Copy link
Member
@benjie benjie commented Aug 31, 2016

Thanks to @taion for implementing graphql-type-json; I've just added it in to postgraphql - seems to work well for my purposes.

@calebmer
Copy link
Collaborator

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 JSON.stringify/JSON.parse work for you? What data are you storing in JSON types? Could you use a composite type instead of a JSON object?

@taion
Copy link
taion commented Aug 31, 2016

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.

@benjie
Copy link
Member Author
benjie commented Aug 31, 2016

I personally just don't like the idea of doing JSON.parse(...) in every single render (often multiple times if multiple components depend on the same field). Also, oddly, for me the stringified JSON from the database comes up as [object Object] in GraphiQL; but I assume that's just me doing something wrong somewhere.

I tried creating custom types to represent the data but PostgraphQL (or, more likely, the pg module itself) was not happy with that when referenced as a computed column. Something like (from memory):

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.

@calebmer
Copy link
Collaborator
calebmer commented Sep 1, 2016

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 JSON.parse argument is a good one, but I'm still not convinced. Also, are you using JSON as truly arbitrary schemaless data or as a method of better organizing data?

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 id. What if the arbitrary JSON has an id property?

I'd love to hear @leebyron and someone from the Apollo team's perspective here (I believe @helfer is a good cc).

@taion
Copy link
taion commented Sep 1, 2016

I haven't had any issues with that so far. Generally we're using JSON in GraphQL for JSONB columns in Postgres.

@helfer
Copy link
helfer commented Sep 3, 2016

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 dataIdFromObject function, which the user provides.

@taion
Copy link
taion commented Sep 3, 2016

Here's the Twitter thread: https://twitter.com/jimmy_jia/status/725715287115456512

@calebmer
Copy link
Collaborator
calebmer commented Sep 3, 2016

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 main.js.

Also, can we copy the implementation instead of adding another dependency? It should be a simple enough implementation.

Finally, tests. Thanks 👍

@taion
Copy link
taion commented Sep 3, 2016

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.

@calebmer
Copy link
Collaborator
calebmer commented Sep 21, 2016

As an update, I’ve fixed JSON serialization here #121. It was always supposed to use JSON.stringify and not just the String constructor. I’m still open to merging this PR if you can put it behind a feature flag 👍

@calebmer calebmer mentioned this pull request Oct 10, 2016
4 tasks
@calebmer
Copy link
Collaborator

I added a flag to enable this feature in PostGraphQL 2. Thanks so much @benjie, @taion, and @helfer for convincing me this is an ok thing to do 😊 👍

To start using this feature download the PostGraphQL 2 beta (#145)

npm install -g postgraphql@next
postgraphql --dynamic-json

@calebmer calebmer closed this Oct 10, 2016
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 this pull request may close these issues.

4 participants
0