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

[RFC] Remove support for Tuple and Variant #104

Closed
Leonidas-from-XIV opened this issue Jul 31, 2020 · 3 comments · Fixed by #158
Closed

[RFC] Remove support for Tuple and Variant #104

Leonidas-from-XIV opened this issue Jul 31, 2020 · 3 comments · Fixed by #158
Labels
rfc Request for Comments on future direction
Milestone

Comments

@Leonidas-from-XIV
Copy link
Member

Current state

Yojson currently supports proprietary extensions to JSON, namely using (x, y) syntax to represent tuples and <Bar:"abc"> for variants.

Proposal & Rationale

JSON is mainly a language-neutral format for information exchange, not much for serialization of data and as such it makes sense to support a language-interoperable set of JSON grammar. As such, outputting data using these features will yield problems with other parsers that do not understand that grammar extension.

As such the suggestion is to remove these variants from the Yojson.{Basic,Safe,Raw}.t type, remove the pretty printing support and remove the parsing code.

Removing the code makes the maintenance easier, is simpler for people to understand (people knowing JSON might be confused why `Tuple or `Variant can be produced) and makes it easier to work on the parser since it can match the grammar of RFC 8259 without fitting in proprietary data types.

Impact

While this is a breaking change, we estimate the impact to be low, since presumably most JSON parsed with Yojson comes from non-OCaml sources that do not use these non-standard extensions.

The most obvious places that might break in the OCamlverse might be the schema generation tools:

  • ppx_deriving_yojson does not use `Tuple nor `Variant, it encodes both tuples and variants as `Lists
  • ppx_yojson_conv does not seem to use `Tuple nor `Variant
  • atd seems to encode things using `Variant (source), but it might be possible to switch to an encoding that's more like what the deriving-plugins do? Is there someone who we could talk on atd's behalf? @rgrinberg maybe?

Roadmap

This would be planned for a 3.0.0 release at the earliest, to give people enough time to discuss the impact and update their code. We could possibly try a rebuild of the revdeps in the duniverse to see how many projects are affected.

@Leonidas-from-XIV Leonidas-from-XIV added the rfc Request for Comments on future direction label Jul 31, 2020
@mjambon
Copy link
Member

mjambon commented Jul 31, 2020

For what it's worth, I apologize for introducing this extension in the first place.

atdgen normally does not go through the Yojson.Safe.t type. However it is used by the user who writes an adapter, which is a late addition to atdgen. Changing the variants in Yojson.Safe.t might break atdgen's runtime library (need to check) but I'm confident it would be easy to fix.

@rgrinberg
Copy link
Contributor

atd seems to encode things using `Variant (source), but it might be possible to switch to an encoding that's more like what the deriving-plugins do? Is there someone who we could talk on atd's behalf? @rgrinberg maybe?

I'm in favor of the change in this PR. I can help change atdgen as necessary.

@craff
Copy link

craff commented Dec 25, 2022

I have no problem with these extensions, espacially that the ~std flag allows standardization.
It is clearly an improvement for Ocaml<->Ocaml communication.
I would also enjoy a clearer separation of Int and Float, in a non standard extension.

But to accept this, you should provide at least javescript library parsing your extension (together with
rescript and typescript binding which would be mostly trivial).

This should be relatively easy using js_of_ocaml.

I think javascript is the main non OCaml target?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Request for Comments on future direction
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants