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

Make jsonm friendlier to jsoo. #20

Closed
wants to merge 3 commits into from
Closed

Conversation

hhugo
Copy link

@hhugo hhugo commented Nov 8, 2021

js_of_ocaml doesn't support general tail call optimization. In particular, cps is not optimized.
jsonm heavily use cps that makes it very prone to stackoverflow with jsoo.
This PR implement trampolines for encoders and decoders when using js_of_ocaml.

Review is easier using patdiff.

@dbuenzli
Copy link
Owner

dbuenzli commented Nov 8, 2021

@hhugo ? Why the hell are you using jsonm in the browser ?

@hhugo
Copy link
Author

hhugo commented Nov 8, 2021

I'm using jsonm because it's used in a "general" libraries (e.g. https://opam.ocaml.org/packages/data-encoding/)

@dbuenzli
Copy link
Owner

dbuenzli commented Nov 8, 2021

I may be a bit grumpy right now, but honestly I'm not super fond of starting to add kludges to support js_of_ocaml in my libraries (especially for a JSON parsing library). I'm also not super fond of the CPS monkey dance happening in this module and would rather see that replaced well placed effects in the future (if any).

@hhugo
Copy link
Author

hhugo commented Nov 9, 2021

I may be a bit grumpy right now, but honestly I'm not super fond of starting to add kludges to support js_of_ocaml in my libraries (especially for a JSON parsing library).

I'm not found of it either, but that the only solution I found, and I found the cost to be relatively low (despite this large diff).

You could argue that jsonm should not be used when targeting js because js already know how to parse json. I sadly don't know how to scale such approach and achieve code reuse. Virtual libraries can help a bit, but doesn't scale well either in my opinion.

@dbuenzli
Copy link
Owner

dbuenzli commented Dec 4, 2024

jsont having been released. I have no plans to further develop jsonm. I'm happy to continue to maintain and bug fix it as it is but it won't receive new developments. I'm also happy to give it away if someone wants to take over.

You could argue that jsonm should not be used when targeting js because js already know how to parse json. I sadly don't know how to scale such approach and achieve code reuse. Virtual libraries can help a bit, but doesn't scale well either in my opinion.

Note that if you use jsont values then it's just a matter of using a different codec according to your context at IO boundaries (e.g. Jsont_bytesrw or Jsont_brr). You can keep all the JSON to OCaml mapping machinery entirely agnostic to the without having to resort to virtual libraries.

So I'm closing this.

@dbuenzli dbuenzli closed this Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants