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

Serialize function #20

Closed
wants to merge 16 commits into from
Closed

Serialize function #20

wants to merge 16 commits into from

Conversation

baransu
Copy link
Collaborator

@baransu baransu commented Oct 5, 2019

Resolves: #20

This PR introduces opt-int GRAPHQL_PPX_SERIALIZATION_EXPERIMENTAL=true flag which adds

let serialize: t => Js.Json.t

function to the generated module. This feature is experimental and breaks some existing features because of it's limitations.

Limitations

  • @bsField is not supported as it's only one way (adding new fields to query). Possible solution - maybe we can work around it by flattening the object, not sure about this.
  • @bsDecoder is not supported because it specifies only decode function. Possible solution: provide @bsDecoder(decodeFn: "", encodeFn: "")
  • Unions have to be exhaustive. While interfaces provide data parsed as non exhaustive, with unions we lost information about fields and typename with Nonexhaustive case. Possible solution - change Nonexhaustive to Nonexhaustive(typename, Js.t('a))
  • Native Reason not supported yet

@baransu baransu added the enhancement New feature or request label Oct 5, 2019
@baransu baransu added this to the 0.3 milestone Oct 5, 2019
@baransu baransu marked this pull request as ready for review October 11, 2019 11:15
@baransu
Copy link
Collaborator Author

baransu commented Oct 19, 2019

I think a good idea would be to enable serialization per a single [%graphql] and not a global feature flag. That way it would allow to gradually introduce it into the project codebase and wouldn't be a breaking change.

IMO best option would be to add support for optional config for [%graphql] allowing to fine-tune generated module. This way it would allow us to introduce new features like #14 more easely

@jfrolich
Copy link
Collaborator

Is this the reverse of parse? This would actually be great to facilitate a correctly typed optimisticResponse. We need to serialize that before passing it to apollo.

@baransu baransu self-assigned this Nov 22, 2019
@baransu
Copy link
Collaborator Author

baransu commented Nov 22, 2019

@jfrolich Thats exactly the idea. It will allow direct cache manipulation keeping all Reason/OCaml types like variants, etc.

@jfrolich
Copy link
Collaborator

Ok. I will incorporate this in reason-apollo-hooks when this lands!

@MargaretKrutikova
Copy link

Would it be technically possible to have the "pure JS" type along with the parsed response type (with bsRecord, bsDecoder etc) available on the ppx?

We are discussing how to improve usage of apollo cache in reason-apollo-hooks and serialize function would be one of them, but also having the type of the objects in cache would allow us to at least avoid resorting to bs.raw for merging cache results.

@fakenickels came up with this idea here, would be really cool to know whether it is techincally possible to implement 😄

@baransu
Copy link
Collaborator Author

baransu commented Dec 13, 2019

@MargaretKrutikova It shouldn't be a problem. We can add any additional fields to the query result. For me, it makes sense to do it only for @bsDecoder as there are no problems with @bsRecord when parsing/serializing in both ways. But in general, I'm rather against it.

The current plan is to provide serialize as an opt-in feature via configuration similar to per query schema configuration. This would allow us to deprecate problematic one-way decorators like @bsDecoder and @bsField for queries with serialize function. What do you think?

/cc @jfrolich @fakenickels

@mbirkegaard
Copy link
Contributor

mbirkegaard commented Dec 15, 2019

Chiming in here (and had this comment over at reason-apollo-hooks)! We use both reason-apollo-hooks and graphql_ppx_re in production and have use cases for interacting directly with apollo (we do both cache updating and optimistic responses). I'm happy to see that things are happening to make this easier.

This would allow us to deprecate problematic one-way decorators like @bsDecoder and @bsField for queries with serialize function.

We use both @bsField and @bsDecoder in queries that we also need to serialise. Would this change make it impossible?

Even having just the pure JS type of queries and mutations available (in addition to the parsed type of course) would be incredibly helpful for us - at least until a (better) solution has landed.

@baransu
Copy link
Collaborator Author

baransu commented Dec 15, 2019

@mbirkegaard I don't want to deprecate @bsField and @bsDecoder but those decorators work only one way so we're unable to serialize parse result with them back to Json.t. Do you use those decorators heavily and it would be hard for you to migrate?

Having a pure JS type seems interesting. It probably could work similar to reason-relay and return enums as strings, etc. Unions and interfaces can be problematic because you cannot mix different types together. What do you think about those?

@mbirkegaard
Copy link
Contributor

mbirkegaard commented Dec 16, 2019

@mbirkegaard I don't want to deprecate @bsField and @bsDecoder but those decorators work only one way so we're unable to serialize parse result with them back to Json.t. Do you use those decorators heavily and it would be hard for you to migrate?

We use @bsDecoder a lot. We only started using @bsField recently after #46 was merged, but it's already becoming a quite useful. We write modules like this:

module Price = {
   type t = 
     | Price(int)
     | PriceRange(int, int);

  module Fragment = [%graphql {|
      fragment price on Item {
         price
         priceRange
         minPrice
         maxPrice
      } 
  |}];

  let decode = obj => {
    switch (obj##priceRange, obj##price, obj##minPrice, obj##maxPrice) {
      | (Some(true), Some(price), _, _) => Ok(Price(price))
      | (Some(false), _, Some(min), Some(max)) => Ok(PriceRange(min, max))
      | _ => Error({j|error decoding price: $obj|j})
    }
  };
};
}

(We use a module with some decoder helpers to build decoding pipelines, but that isn't really important here)

The fragment is then used in other queries/types via @bsField and use Price.decode in that place (afaict we can't use the decoder directly on the fragment, i.e. fragment price on Item @bsDecoder(fn: "decode)). The decoder for these other types get to decide how they respond to Ok and Error.

We want to get to the point where we only define once per domain type (like price) how to ingest data from GraphQL and turn it into a type for the frontend. There's still a way to go, but it already looks pretty promising.

Having a pure JS type seems interesting. It probably could work similar to reason-relay and return enums as strings, etc. Unions and interfaces can be problematic because you cannot mix different types together. What do you think about those?

I must admit that I don't know how reason-relay does it. I looked through their docs, but can't seem to find the things that you're referring to?

Or do you mean how I think unions and interfaces should be handled? Hmm... yeah. Need to think about that - I don't really have a good answer for that yet.

Writing encoders, either for whole queries or with a @bsEncoder type directive would be quicker and less error prone if there was a JS type to target.

Come to think of it, is this the right place for me to bring these things up? -- Seems it should be in an issue rather than on this PR.

@baransu
Copy link
Collaborator Author

baransu commented Jan 6, 2020

@mbirkegaard If you have suggestions and feature requests not directly related to this PR, please open an issue. As far as I understand your problem is lack of @bsDecoder on fragments for example, as it would simplify your workflow when handling e.g Price fragment as in your example?

I think the best option would be to deprecate @bsDecoder when adding serialize function and introduce two-way decorator: @bsSerde(decode: "decodeFn", encode: "encodeFn") (name up to discussion). It would allow us to still have this functionality and where encode is no option, it would force users to decode later, not involving graphql_ppx into it.

Still, not sure how to handle @bsField 😕

@mbirkegaard
Copy link
Contributor

mbirkegaard commented Jan 6, 2020

@mbirkegaard If you have suggestions and feature requests not directly related to this PR, please open an issue.

Will do! It started out being kinda related to this, but then I got carried away 😄

@jfrolich
Copy link
Collaborator

jfrolich commented May 2, 2020

This shipped in another form in #101 (same functionality).

@jfrolich jfrolich closed this May 2, 2020
@baransu baransu deleted the serialize_fn branch May 6, 2020 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants