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

Put Ojs_exn inside Ojs module (breaking change !) #126

Closed
wants to merge 2 commits into from

Conversation

mlasson
Copy link
Member

@mlasson mlasson commented Dec 22, 2020

This module does not seem to be used by public libraries on opam.
I don't think this will break a lot of foreign code.

This module does not seem to be used by public libraries on opam.
I don't think this will break a lot of foreign code.
@mlasson mlasson linked an issue Feb 1, 2021 that may be closed by this pull request
@mlasson mlasson changed the title Break compatibility to wrap modules Put Ojs_Exn inside Ojs module Feb 1, 2021
@mlasson mlasson changed the title Put Ojs_Exn inside Ojs module Put Ojs_exn inside Ojs module (breaking change !) Feb 1, 2021
@alainfrisch
Copy link
Collaborator

We need to be careful, since forcing to link Ojs_exn makes it more difficult use gen_js_api together with js_of_ocaml stdlib or brr. This is discussed in dbuenzli/brr#2.

The best solution would probably be to have js_of_ocaml come with a "core" interop stdlib that registers the exception to the runtime system, and possibly expose primitives of the runtime system, allowing higher level FFI and binding system to live well together.

Short of having something like that, it seems too dangerous to force linking Ojs_exn. One possibility to "fix" #126 could be to have Ojs_exn be its own library, but it's a bit silly. Anyway, I don't think this PR should be merged, so I'm in favor of closing it. @mlasson, do you agree ?

@mlasson
Copy link
Member Author

mlasson commented Feb 4, 2021

Yes, I was arriving at the same conclusion.

@mlasson mlasson closed this Feb 4, 2021
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.

Change Ojs_exn to Ojs.Exn
2 participants