Unify Json and ppx runtime modules

[andreypopp]

Closes #36.

[anmonteiro]

Ppx_deriving_json_runtime is still reading Json.Decode.error which seems to be gone?

[jchavarri]

This is looking great @andreypopp !

I fixed the tests (don't ask how 🙈 ) in #46. For now I use the deprecated APIs (also helpful to see how users will have to adapt their code). Once they are removed we could update the tests to use the new APIs because they're quite exhaustive.

As can be seen in the PR, some of the JSON error messages were more detailed before. We should bring these details back somehow, do you think it'll be too hard with the PPX approach?

[jchavarri]

This is not DecodeError anymore.

[jchavarri]

Unexpected_variant currently always sends "unexpected variant" in the string payload. Should this payload be removed?

[andreypopp]

I think I've wanted to include the tag name there.

[jchavarri]

I think #46 is ready:

• updates error messages so they show the value received in the JSON, not only the failure
• improve errors in some cases like objects with new information about which key is failing to be parsed
• remove duplication in the browser ppx runtime code, reusing functions from Json module
• remove DecodeError exn altogether to make sure that any code relying on it fails to compile, as it should use now Oj_json_error exn
• fix ci tests
• silence deprecation alerts in examples

Re: the last point, maybe we should update the examples code to use the new functions, but we can probably do that once they're removed completely?

If you think #46 is good to merge here, feel free to do so. The only things missing would be changelog entry and we should probably test some existing package or project that uses this library to make sure everything works as expected (melange-atdgen-codec-runtime is a good candidate).

[andreypopp]

@jchavarri thanks!

I've merged your PR and then removed ppx_deriving_runtime libs completely, instead melange-json and a (new) melange-json-native is used as runtime libs instead.

[jchavarri]

Cool! I started taking a look at upgrading melange atdgen codec runtime, but it seems that library blended DecodeErrors from this lib with its own DecodeErrors instances 😕 so having a hard time fixing the tests. Will see if I can take another look this week and report back with more details about how the migration looks like from the user perspective.

[jchavarri]

here's a branch of melange-atdgen-codec-runtime that builds with latest main: ahrefs/melange-atdgen-codec-runtime#53. hopefully not many more changes are needed to make it work with this branch.

[lessp]

Any blockers for this to go in? 👼

[andreypopp]

Needs a rebase on top of latest changes in main.

[jchavarri]

The last commit merged main into this branch, had to update a few things to accomodate the changes in #47 into this branch.

Tried to use @EmileTrotignon's new error system as much as possible. One small change in error msgs can be seen in Json_decode_test:

• Before: Expected array of length 2, got array of length 1
• Now: expected tuple as array of length 2 but got [4]

I think new format is better as it shows the actual value, but curious what you think.

After this, I'll check the melange-atdgen-runtime-codec branch to use the latest commit. A couple more things:

• Noticed there are no decoding tests for result types
• I keep high hopes that we can get rid of Jest soon 😄 and use cram tests instead. Not sure why, but the errors that Jest reports now are useless (see example shown below), so one has to add console.logs in the generated js file to understand what's going on.

  ● tuple2 › too small

    MelangeError: Json__Errors.Of_json_error/8

      795 |                         return Jest.pass;
      796 |                       }
    > 797 |                       throw new Caml_js_exceptions.MelangeError(exn.MEL_EXN_ID, exn);
          |                             ^
      798 |                     }
      799 |                     throw new Caml_js_exceptions.MelangeError(exn.MEL_EXN_ID, exn);
      800 |                   }

      at new MelangeError (src/__tests__/test/node_modules/melange.js/caml_js_exceptions.js:28:21)
      at src/__tests__/test/src/__tests__/Json_decode_test.js:797:29
      at Object._1 (src/__tests__/test/node_modules/melange.js/curry.js:31:12)
      at Object.<anonymous> (src/__tests__/test/node_modules/melange-jest.jest/jest.js:216:24)

[jchavarri]

I updated ahrefs/melange-atdgen-codec-runtime#53 with this branch and everything seems to be ok.

Added a changelog entry and also a workaround to improve the tests, until we get rid of jest. From what I can see the only things missing are:

• examples and tests are still using old Decode/Encode modules, but I guess this is fine until they get removed altogether
• there was a comment about Unexpected_variant, not sure if we want to do something about it

otherwise, it looks good to mark as ready for review.

[jchavarri]

@EmileTrotignon do you know how the error handling can print the OCaml expression name, rather than the JavaScript runtime value?