zth/rescript-relay

Array scalars are not well behaved.

ValdemarGr opened this issue · 14 comments

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

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

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

zth commented

@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
Yes this fixes the issue, thank you!

@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

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

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

@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

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

zth commented

@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).

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

zth commented

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

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

zth commented

@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!