teamwalnut / graphql-ppx

GraphQL language primitives for ReScript/ReasonML written in ReasonML
https://graphql-ppx.com
MIT License
257 stars 53 forks source link

Custom Scalar parsing/serializing not working in InputObjects #272

Open illusionalsagacity opened 2 years ago

illusionalsagacity commented 2 years ago

Reproduction: https://github.com/illusionalsagacity/graphql-ppx/commit/9630e024722b813603161dbfb8c181f73f7b4efd#diff-dccca68100be513fb2d07560888ab2c0785e6033bc6babd303fb6bfe07810f1dR52

tests_bucklescript/__snapshots__/Generate_Apollo_scalarsInput_re.bda5f930.0.snapshot line 52

when using customFields, the Scalar types in input objects do not seem to be using those custom parsers and instead show up as option(Js.Json.t)

Looks like this was just missing from the test cases?

a-c-sreedhar-reddy commented 2 years ago

Hi @illusionalsagacity, I have not used GraphQL much. So please correct me if I am wrong.

According to the GraphQL spec the client must serialize the value of the scalar type. So it makes sense of graphql-ppx accepting a type Option(Js.Json.t). We might have to to use something like https://rescript-lang.org/docs/manual/v8.0.0/api/js/json#number and pass the result as input.

image

illusionalsagacity commented 2 years ago

My expectation was that since we've defined a customFields module for this DateTime scalar, it would be handled by the PPX for both responses and inputs; otherwise why does the module require a serialize function for the custom scalar handling?

https://github.com/reasonml-community/graphql-ppx/blob/ebb140459493f3f440f0c34f7ff3f34632fa1dad/tests_bucklescript/operations/customTypes.re#L48

https://graphql-ppx.com/docs/custom-fields

a-c-sreedhar-reddy commented 2 years ago

Oh graphql-ppx allows us to provide a decoder! Great. Then I think this is bug.

jfrolich commented 2 years ago

Custom fields have not been implemented for input objects indeed! I definitely would welcome a PR if you are up for it.

illusionalsagacity commented 2 years ago

Custom fields have not been implemented for input objects indeed! I definitely would welcome a PR if you are up for it.

Any idea on where/what would a starting point for this would be? Looking around in src/base/result_decoder.re at the moment.

Zimmi48 commented 2 years ago

When using the GitHub GraphQL API, there used to be a workaround: it was possible to use String instead of custom scalars (like GitObjectID) because the types were not enforced. Following a recent update of one of their dependencies, this workaround no longer exists, making the issue more pressing. Check coq/bot#203 for details.

jfrolich commented 2 years ago

Custom scalars should be just the JSON type. So if it's a string natively, you need to convert it to a JSON type (YoJson on native, Js.Json.t on ReScript).

jfrolich commented 2 years ago

It's actually not related to this issue, and should just work.

Zimmi48 commented 2 years ago

I meant custom fields in the context of input objects (like you said above had not been implemented yet).

jfrolich commented 2 years ago

I meant custom fields in the context of input objects (like you said above had not been implemented yet).

Custom fields are just an ergonomic way to automatically convert custom scalars (or other custom fields). But you can use custom scalars in input objects, you just have to convert to a Json type.