Reactive dom about eliom HOT 36 CLOSED

I like this a lot, I'm going to look at it closer during next week but a few questions :

• You seem to retraverse the whole html tree at each update and lift everything as a signal. How is it performance-wise compare to manual singular update ?
  If we really need this lifting, can we do it only once (I could be mistaken, but it seems you're doing it at each update).
• Why didn't you use the same method as D and F : build a custom Xml module as base and call the Html5_f.Make functor ? This would give us the whole html in the R module easily and we could reuse the Xml module for Svg (and it's easier to maintain).
• How do this handle changing the type of the subnode (from the phantom-type point of view) ?

from eliom.

• Really not sure I understand your question (misunderstanding about react?). There are no html tree traversal. It rely on React to update what needs to be updated. I never ran any benchmark but never saw performance issue. There exists a memory leak issue as React relies on Weak value (javascript doesn't have such a support). I have been willing for a long time to ask @dbuenzli if he had any solution for this.
• R module is quite different from D/F. Its only purpose it to allow signals into regular tree (ie: node React.signal inside regular nodes). This signature doesn't make any sense inside R (http://ocsigen.org/tyxml/api/Html5_sigs.T).
• The following works (Does this answer your questions ?)

type t = Case1 | Case2
let case_signal,set_signal = React.S.create Case1
let real_dom =
  let dom_sig = React.S.map (function
    | Case1 -> div []
    | Case2 -> p []
  ) case_signal in
  div [ h1 [pcdata "title"]; R.node dom_sig; p [ pcdata "footer"] ]

dom_sig has type [> Div |P ] Eliom_content.Html5.D.elt React.signal

from eliom.

There are but best would be to petition the standard bodies so that they give us a sensible semantics for weak references.

from eliom.

• I did look very quickly so I'm probably wrong. I will look closer.
• Well, you did reimplement some attributes into the R module and reimplement all of them without using the functor is a terrible idea.
  Also, having : R.div : 'a elt list signal -> [> Div] elt(sort of) sounds like a good idea for me and should be doable. We also of course need the pseudo lift operator (the one you calledR.node`) on top of that.
• This does answer my question, and in a quite satisfying way ! :)

from eliom.

R.div : elt LIST SIGNAL -> div elt could be a good idea.
You cannot use Html5_f.Make out of the box for that as the signature is val div : elt LIST -> div elt.
The same problem with attributes (number doesn't match with number signal, etc ..).
But you're right about the terrible idea of reimplementing everything.
It seems to me that we need to change Tyxml for that.

The core of R could be only this :

R.node : 'a elt signal -> 'a elt
R.attr: 'a attrib signal -> 'a attrib

And the following could be generated with help of some Tyxml functor

R.div : elt LIST SIGNAL -> div elt
....
R.a_title : string signal -> title attrib
....

from eliom.

This was precisely my idea. We will need to fiddle with tyxml's functor but I think it's doable.

from eliom.

I've just created a new branch so it's easier to work on it.
https://github.com/ocsigen/eliom/tree/reactive

from eliom.

@balat, you can give it a try easily now :)

from eliom.

ok I will try :) Thank you!

from eliom.

I implemented modifications in tyxml to allow to wrap element in a monad here. Basically, there is now a MakeWraped functor that take as extra first argument a monad.
I had to change the default interface a little bit, so every Module implementing the Xml signature must now have a type 'a wrap. The module Make doesn't take any monad and expect 'a wrap = 'a.
There is also an awful lot of misc changes to make the lifting possible for every nodes and attributes.
I modified both Html5_f and Svg_f.

I have modified eliom here to compile against this version of tyxml, no modification apart from adding the relevant typing information in the relevant places.

If someone could test it against a large eliom website and If you have remarks, it would be perfect !

from eliom.

Works great (except missing installation of Xml_wrap.cmi).
here is a quick implem of reactive dom (https://github.com/besport/eliom/commit/9f9e382a45d47a282dbcc7a7845b6b48922bfe9c).
we should probably move some modules (Xml_w & Xml_wed) in other dir.
Thanks for your work on tyxml

from eliom.

@chambart said he has some modification to do on your implementation, so we're going to wait for that before merging it.
I will merge my stuff after some more testing.

from eliom.

I've merged everything into https://github.com/ocsigen/eliom/tree/reactive so @chambart can work on it.

from eliom.

I didn't pay attention earlier, but there is an important issue with my change in tyxml : in error reporting, every single xml elements is wraped in an additional type constructor, which makes everything even more confusing than it already is, especially for beginners. Same for the documentation.
I'm really not happy about that. The additional type is possible to avoid but imply some code duplication (the Xml_sigs.T signature would be duplicate and that's all, I think).
There is also the brand new option in 4.01, -short-path, that we could activate by default in eliom, but It wouldn't help with the documentation. I think we should make this option activated by default in eliom anyway.

What do you think ? Does this simplification worth the (admittedly small) code duplication ?

from eliom.

wouldn't it be possible to use pa_include used in eliom?

from eliom.

Maybe it would be a good idea to separate pa_include from eliom

from eliom.

any update on this ? did @chambart had a look ?

from eliom.

@Drup, any news ? It would be great to have this integrated with the next release of eliom. Is there any issue for this ?

from eliom.

I can't do anything more, sorry. I'm waiting for @chambart as you are, and I'm starting to be quite impatient too :/

from eliom.

Indeed this is the very last thing to implement in Eliom 4. And is becoming urgent. Unfortunately I don't have time to work on this myself in the next weeks, and @chambart seem busy too ... But may be the first step to progress is to write exactly what is to be done (@chambart I guess, with @Drup? Can you do that?). If it has enough details, may be someone else can take in charge the implementation? (@hhugo ? or myself?)

from eliom.

I will try to write a comprehensive documentation of my changes in tyxml during the week end. It has lie around in my todo list for far too long.
About the implementation in eliom, to be honest, I have no idea what changes @chambart wanted to do exactly. I don't really get what's going on in this part of eliom anyway ...

Also, we need serious testing for this feature, and anyone can do that, I think.

from eliom.

Yes once it is implemented I will test it in xprime asap.

from eliom.

what about merging the actual implem now, so everyone can give it a try ?

from eliom.

Give me a day to check than everything is fine on the tyxml side, and to document this stuff. After that, as long as there is no blocking bug for regular dom usage, I think it's a good idea.

from eliom.

I've rebased the changes into a single commit, might need some cleaning #39
I won't bother you anymore for few weeks; I hope I'll have nice surprise when I get back from holidays ;)

from eliom.

@hhugo from my point of view, we are ready ! @jpdeplaix tested on cumulus and I tested on evePI, it doesn't seems to break anything and everything compile fine. I updated my branch on tyxml's master and added some documentation.
If someone wants to test :

opam pin -f tyxml "https://github.com/Drup/tyxml.git#superfunctor"
opam pin -f eliom "https://github.com/ocsigen/eliom.git#reactive2"
opam reinstall tyxml

from eliom.

Thanks a lot for your help.

I cannot test anything right now but I would merge both branches and do extra testing before the next release.
We should maybe tag the R module as experimental (in the documentation).

Have you done anything wrt your previous concern (type error being harder to understand). Code duplication is not a big problem as long as it stay small (Xml_sigs only?)

from eliom.

I think we will roll at it's now, and see if people find it confusing. If so, I will change it.
In the error messages, this goes away if you have the -short-path option enabled. About the documentation ... it's meld into star, unary and all that stuff, so it's less visible than I though.

And anyway, it's easy to change, we can do it later.

from eliom.

So I've juste read your new patch. So the overall code seems ok now. I did'nt compile test or
generate the documentation, so I cannot say for sure, but:

• be careful, generating correct documentation for functor application can be tricky.
• please avoid reindenting when you submit a patch for review, it doesn't help. If it is needed
  to have something coherent at the end: do it in 2 patch. One with the real modification, one with
  indentation.

About the code:

• now that there is no 'node signal -> node' function, there is no need for the duplication of the rebuild part.
  drop the part in rebuild_node', and just call raw_rebuild_node on the React.S.value of the signal.
• the raw_rebuild_node function shouldn't go throught the list each time a child is updated:
  just replace the changed node, don't map on the whole list.

I discused a bit with Vincent and I see more precisely what could be usefull on server side, and it don't seems
too complicated. Don't bother with that side until the right interface has been found. The idea whould be to
instanciate the XML functor with a type wrap being React.signal client_value.

from eliom.

And a last thing: please add documentation. Both an ocamldoc part and a manual part.

from eliom.

@chambart
1 - S.value doesn't always succeed

Warning. If executed in an update cycle may return a non up-to-date value or raise Failure if the signal is not yet initialized.

2 - I don't remenber the reason for going through the list of all children. But it was in purpose to fix another issues. might not be needed anymore.

from eliom.

I prefer the small risk of miss-using S.value than the ugly delayed computation by waiting: you probably can't get there by running an handler on a uninitialised signal and if you get the previous value of a signal during the update cycle, it won't mater since it will be updated a bit later by the map you just registered.
And please, when you do that kind of things, use Lwt.pause instead. You absolutely don't want to hand back the control to the browser before finishing running.

from eliom.

new pr : #42

from eliom.

I rewrote the code and made a new PR. Can you have a look ?

from eliom.

About server side reactive nodes

The goal is to be able to generate the reactive nodes on server side.

I suggested that the implementation should not be too difficult, because we basically just need to generate D nodes and send a client value that will update them. Function take client values of type React.signal.

R.div, R.p, ...

This is easy for example for R.div:

module R = struct

  let div signal_client_value =
    let v = D.div [] in
    let _ = {unit{
      ignore (React.S.map (fun l -> Manip.replaceAllChild %v l) %signal_client_value)
    }}
    in
    v

end

(full code at the end if you want to try).

R.pcdata

For pcdata, it's more difficult, because, if I'm right, we don't have D.pcdata (or more precisely, D.pcdata behaves like F.pcdata). @hnrgrgr ? @chambart ?

Attributes

Idem for attributes. We don't have D.a_style I guess?

R.node

To implement R.node, we need to insert something in the page, and change it later??

Full code of the example:

Just copy paste in an Eliom module and insert make_page somewhere in your page.

{client{
let split s =
    let len = String.length s in
    let rec aux acc = function
      | 0 -> acc
      | n -> aux (s.[n - 1] :: acc) (pred n)
    in aux [] len

let make_color len =
  let d = (len * 10) mod 255 in
  Printf.sprintf "color: rgb(%d,%d,%d)" d d d

let title,set_title = React.S.create "initial"
 }}

let title = {string React.signal{ title }}
(* title : string React.signal client_value *)

let title_style =
  {string React.signal{ React.S.map make_color (React.S.map String.length title) }}
(* title_style : string React.signal client_value *)

let content : Html5_types.div_content_fun elt React.signal client_value =
  {Html5_types.div_content_fun elt React.signal{
    React.S.map ( fun title ->
      let l = split title in
      F.div (
        List.map (fun c ->
          F.p [F.pcdata (Printf.sprintf "%c" c) ]
        ) l
      )
    ) title
  }}

let div_content =
  {Html5_types.div_content_fun elt list React.signal{
    React.S.map (fun title ->
      let l = split title in
      List.map (fun c ->
        F.p [F.pcdata (Printf.sprintf "%c" c) ]
      ) l
    ) title
  }}

let input () =
  let ii = D.Raw.input ~a:[a_input_type `Text] () in
  let _ = {unit{
    Lwt_js_events.(async (fun () ->
      keyups (To_dom.of_element %ii) (fun _ _ ->
        let s = Js.to_string ((To_dom.of_input %ii)##value) in
        set_title s;
        Lwt.return ())
    ))
  }} in
  ii

module R = struct

  let a_style signal = a_style ""
    (* let v = D.a_style "" in *)
    (* let _ = {unit{ *)
    (*   ignore *)
    (*   (React.S.map *)
    (*      (fun s -> (To_dom.of_attribute %v)##data <- Js.string s) %signal) *)
    (* }} *)
    (* in *)
    (* v *)

  let pcdata signal =
    let v = D.pcdata "aa" in
    let _ = {unit{
      ignore
      (React.S.map
         (fun s -> (To_dom.of_pcdata %v)##data <- Js.string s) %signal)
    }}
    in
    v

  let div signal =
    let v = D.div [] in
    let _ = {unit{
      ignore (React.S.map (fun l -> Manip.replaceAllChild %v l) %signal)
    }}
    in
    v

end

let make_page () = D.div [
  input ();
  D.p ~a:[R.a_style title_style] [R.pcdata title];
  R.div div_content;
  (* R.node content *)
]

from eliom.

Some ideas of implementation tricks to solve these problem (after discussion with @chambart):

• We can implement R.pcdata as D.span, with something that will change dynamically the span into a pcdata
• R.node could be D.span + something that will replace it on startup
• For attributes, we could generate special attributes and rewrite them during existing tree traversal on startup. e.g.: let a_style = a_data "__eliom_style" "ae574673Af8976bce98797"

@hhugo do you want to give it a try?

from eliom.