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

Handling callbacks in Eio+Js #534

Closed
wants to merge 18 commits into from
Closed

Handling callbacks in Eio+Js #534

wants to merge 18 commits into from

Conversation

balat
Copy link

@balat balat commented May 30, 2023

I'm opening this draft PR (not to be merged) to discuss how it is possible to use Eio in callbacks (JS event handlers ...).
It is built on top of @patricoferris ' PR #405 and adds just one commit with a basic solution to this problem.

The idea:

  • Each callback must be wrapped using [Eio_browser.wrap_callback].
  • The wrapper will send the function to be executed to a main loop (through a stream) that must be run inside [Eio_browser.run]

Drawbacks:

  • It won't work if you forget the wrapper
  • This introduces a yield (I think) before each handler

Feel free to comment and propose better ideas (global switch?).

@patricoferris
Copy link
Collaborator

Thanks @balat ! This is an important issue to address in any JS backend for Eio. This implementation looks sensible, I had originally raised concerns about the switch in the run_callbacks but I think actually it is okay because the fibres do in fact yield to other fibres. run_callbacks just never returns which is fine.

To test that I had a go at making the example a little more complicated in 2173aa7 which:

  • Adds another little async loop adding DOM elements every second
  • Calls run_callbacks internally so users don't have to decide to do that themselves, if they don't use callbacks I can't see that adding much of an overhead because it'll just be a suspended fiber.
  • Adds an analogous wrap_callback for Brr but directly with the listen function which I think is quite nice because the user just has to change any Brr.Ev.listen to Eio_browser.listen and their callback can now perform effects.

(Please don't take me changing jsoo to brr as hostile, I just know it a lot better!)

@balat
Copy link
Author

balat commented Jun 1, 2023

Thank you. Yes of course it's much better if the main event loop is always running.

About Brr: no problem at all. I just don't know how it works, that's why I used the standard js_of_ocaml library instead. We had in mind to build a version of the browser api without object types (using gen_js_ai) but I guess that's what Brr is.
I'm just wondering whether we want the dependency to Brr to show up in Eio's interface (forcing people to use Brr themselves). Eio should remain as neutral as possible.
Or we could decide to make Brr the recommended library if it's complete enough and makes consensus amongst users and js_of_ocaml developers.
@vouillon @hhugo what do you think?

@balat
Copy link
Author

balat commented Jun 1, 2023

Can we use Eio promises instead of Fut?
May be put all Brr related interfaces into a separate module?

@patricoferris
Copy link
Collaborator

I do much prefer the non-object approach to writing OCaml in the browser. Really all we need is what amounts to Brr's Jvmodule or gen_js_api's Ojs. This would be something that would be nice to agree upon and push upstream into js_of_ocaml perhaps. I believe they really are the same and converting between them would be just the identity function with the types being changed, but it would be quite nice not to even need that.

Perhaps we could vendor our own version of this and just use that for doing any browser work we need to do in the internal Eio_browser backend and then provide additional support for specific libraries with something like eio_browser.brr.

Can we use Eio promises instead of Fut?

The way the code is structured right now is that we return a Javascript promise from the main function because we don't really know when the program is finished (someone might click on a button in ten minutes!). We can't use Eio promises at that point because we are outside the handler and awaiting would raise unhandled effect.

I just realised that with the addition of run_callbacks I don't think we'll ever exit so perhaps we don't even need a promise anymore?

@polytypic
Copy link
Contributor

BTW, sometimes browsers, especially mobile browsers, require certain actions to happen during the callback. I've run into this myself while developing a web app. I don't recall the exact details anymore, but it might have been something like e.g. making a certain input element active/focused or scrolling the page in response to a click or touch or some such event. So, what happened there was that my code performed the effect after the callback had returned and then the browser simply didn't perform it. So, while this kind of wrapping can be useful, it is not correct to simply wrap all callbacks.

@talex5
Copy link
Collaborator

talex5 commented Feb 26, 2024

Superceded by #680.

@talex5 talex5 closed this Feb 26, 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.

4 participants