experiment with tyxml #1
Conversation
anuragsoni
force-pushed
the
try-tyxml
branch
5 times, most recently
from
a8256c1
to
47f1036
Compare
tyxml/snabbdom_tyxml.ml
Outdated
| List.fold_left | ||
| (fun (events, attrs) t -> | ||
| match t with | ||
| | `Attr name, v -> events, (name, v) :: attrs |
There was a problem hiding this comment.
If I understand snabbdom's API right, you might have some issues with attributes such as data-foo and others. You probably want to recognize those specially.
Also, you should be able to extend your attribute type and provide all the fancy new things provided by the library as "general" (no type constraints) attributes.
There was a problem hiding this comment.
That is true. I was just testing with some simple attributes and event handlers at the moment. snabbdom seems to support a variety of attributes so i'll need a nice way to work with those both in the core library and the tyxml layer (i don't want to have people worry about creating Jv.obj's and deal with javascript arrays and strings.
I'd also want to deal with event handling attributes a little better. snabbdom uses event names likeclick and tyxml generates onclick and so on?
provide all the fancy new things provided by the library as "general" (no type constraints) attributes.
This sounds reasonable.
There was a problem hiding this comment.
For events, I would move the name mangling in the *_event_handler_attrib functions themselves, the rest is fine. For other attribs, you can use to_attrib to locally (and carefully!) break the abstraction and make the attributes you want.
tyxml/snabbdom_tyxml.ml
Outdated
| include Xml_vnode | ||
|
|
||
| let node ?a name children = | ||
| Vnode.make_node "svg" Jv.null [ Vnode.make_node name (make_attrs a) children ] |
There was a problem hiding this comment.
That's incorrect, if you do g (g ...) it will yield <svg><g><svg><g>......
According to https://github.com/snabbdom/snabbdom#svg , you don't actually need to do anything.
There was a problem hiding this comment.
Good catch! I totally missed the val svg : ... created by the tyxml functor 🤦🏼♂️ I just needed to override the svg function generated by functor as it adds some namespace attributes, and it looked like it was clashing with the svg attributes that snabbdom adds for the svg element.
There was a problem hiding this comment.
Hmm, you still shouldn't need to override this svg function. This one is made to create standalone SVG document.
There was a problem hiding this comment.
Hmm, it looks like tyxml automatically adds the namespace attributes for the svg tag. So i'd need to override the attribute. Without that I get a namespace related error when trying to create the node via snabbdom (my guess is snabbdom doesn't like the pre-existing namespace attributes added by tyxml). This is what i get from tyxml
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
^ is what tyxml creates for the svg node which looks consistent with https://github.com/ocsigen/tyxml/blob/8551b673857fdc1defebe7c502ffcb920f04009e/lib/svg_f.ml#L738-L744
There was a problem hiding this comment.
If snabbdom doesn't like those attributes, treat them specially to remove them. Html.svg is not the only place they can be introduced.
We can use the same [h] function for both html and svg nodes. We just need to ensure that we don't send in namespace attributes for svg since snabbdom takes care of that for us.
example/main_tyxml.ml
Outdated
| ; a_fill (`Color ("yellow", None)) | ||
| ] | ||
| []) | ||
| svg |
There was a problem hiding this comment.
That's the wrong svg function, so to speak. It's made for standalone SVG document.
What you want is "SVG in HTML", aka Html.svg.
| end | ||
|
|
||
| module Svg = Svg_f.Make (Svg_vnode) | ||
| module Html = Html_f.Make (Xml_vnode) (Svg) |
There was a problem hiding this comment.
You forgot the .mli/signatures for Html and Svg. The signatures are essential to make the tyxml typing works. See https://ocsigen.org/tyxml/latest/manual/functors#sig for the explanations.
This is why your svg example code was wrongly accepted, it shouldn't have been. :p
There was a problem hiding this comment.
So i added the mli module, but I don't see an error when using the Svg.svg example i had before. I also tried it with regular tyxml under utop and tyxml seems to accept it without issues. I'm also not sure I'm doing the right thing here with the signatures. The docs recommend not exposing the +'a elt = Xml.elt equality, but without that I won't be able to forward the tyxml output to the snabbdom patch function (the core library expects to work with vnodes)
There was a problem hiding this comment.
You should most definitely not expose that equality! It bypasses tyxml's typing.
When you want to obtain some Xml.elt (in your case, to render it), you need to use Html/Svg.toelt. I suggest you expose a function that renders an 'a elt directly.
There was a problem hiding this comment.
You should most definitely not expose that equality! It bypasses tyxml's typing.
The newest commits should ensure I don't expose these in the mli. I'm working via toelt at the moment. I think eventually i'll try to have everything needed to use snabbdom re-exported in the tyxml specific package too so it exposes its own init/patch functions that work with tyxml elements and the user can stick to just working with a -> this is now done. There is now an Snabbdom_tyxml module.init function in the tyxml module that works with 'a elt directly.
anuragsoni
force-pushed
the
try-tyxml
branch
2 times, most recently
from
7bdc2fc
to
fafe2cd
Compare
|
I don't have any more remarks! :p Congrats for this, it looks really nice. |
|
@Drup Thanks for the review 😄 |
Nop, happy to help, and to see one more tyxml instance #takingovertheworld :D |
No description provided.