zth / rescript-relay

Use Relay with ReScript.
https://rescript-relay-documentation.vercel.app/docs/getting-started
338 stars 53 forks source link

Array scalars are not well behaved. #407

Closed ValdemarGr closed 1 year ago

ValdemarGr commented 1 year ago

Arrays don't behave well when used as the underlying type of a scalar. If an array occurs as the backing type for a scalar, it's contents all become 0 even though the scalar's serializer is invoked.

A test that reproduces can be found here: https://github.com/casehubdk/rescript-relay/blob/59e0a1f9a3b9bc42ba473fb15710c40bb9da4044/packages/rescript-relay/__tests__/Test_customScalars.res

We have deployed the following fix internally: https://github.com/casehubdk/rescript-relay/blob/59e0a1f9a3b9bc42ba473fb15710c40bb9da4044/packages/rescript-relay/src/utils.js#L260

Our fix only handles a special case of the problem; when the array occurs inside of an object. I don't understand the code in utils.js very well, so our fix does not solve the problem of toplevel array scalars.

Here is the test output: image

zth commented 1 year ago

Thank you for the report! This part of the API is currently a bit buggy. cc @tsnobip who's working on correcting similar issues.

tsnobip commented 1 year ago

yeah hopefully such behavior will soon be a thing of the past!

zth commented 1 year ago

@ValdemarGr would it be possible for you to try 0.0.0-fix-custom-scalars-104-8868877b? I've attempted to fix this in the current structure https://github.com/zth/rescript-relay/pull/434 (as we wait for a new structure to eventually ship).

ValdemarGr commented 1 year ago

@zth Yes this fixes the issue, thank you!

ValdemarGr commented 1 year ago

@ValdemarGr would it be possible for you to try 0.0.0-fix-custom-scalars-104-8868877b? I've attempted to fix this in the current structure #434 (as we wait for a new structure to eventually ship).

@zth I have conducted some more testing and found that this patch introduces a bug on the decoding side instead; Now when decoding an array of scalars, the scalar decoder is not invoked for every element in the array, but instead for the whole array

image

module BigDecimalScalar = {
  type t = Big.t
  let parse: Js.Json.t => t = json =>
    switch Big.t_decode(json) {
    | Ok(b) => b
    | Error(err) =>
      Js.Console.error3(`big decimal from string decoding failure`, err, json)
      Js.Exn.raiseError(`failed to parse big decimal from string`)
    }
  let serialize = Big.t_encode
}
zth commented 1 year ago

Right, thanks! I think that should be a fairly easy fix. I'm going traveling for a week soon, but I can hopefully sort it out after that.

zth commented 1 year ago

@ValdemarGr could you paste a generated file where this problem happens?

ValdemarGr commented 1 year ago

@zth https://gist.github.com/ValdemarGr/5de0cd9c1962fe6fad4cba60cef28f37 This is a minimal re-production in our app. Note that some names have been redacted, I hope that is fine.

zth commented 1 year ago

@ValdemarGr thank you! That's fine, no problem.

zth commented 1 year ago

@ValdemarGr would it be possible for you to test this one? 0.0.0-fix-custom-scalars-9724b22f

It's from master, so you'll probably need to do a few additional upgrades to make it work (jsx v4 comes to mind).

ValdemarGr commented 1 year ago

@zth I managed to get the project running in jsx v3 compat mode and both encoding and decoding looks fine now.

zth commented 1 year ago

@ValdemarGr that's excellent! So with that, you have no current outstanding issues after these fixes?

ValdemarGr commented 1 year ago

@zth I think that fixes our problems, thank you very much :). Feel free to close the issue.

zth commented 1 year ago

@ValdemarGr concluding this is enough duct taping for now then, until we redo this entire conversion setup (which is planned, and is in dire need of an overhaul).

Thank you for your reporting and testing, I really appreciate it!