ppx: Remove @@@react.dom + transform lowercase elements to Dsl functions

[jchavarri]

Continues integration work after @glennsl work in #119.

• Update ppx to generate fun application calls to element names, e.g. <div/> is transformed into div [||] [].
• Removes @@@react.dom attribute as it's not needed anymore
• Updates ppx mapper to not be contextual
• Removes dom_tags check in ppx, as there's no need to validate lowercase elements at syntactic pass anymore

TODO

• [x] Fix dangerouslySetInnerHtml
• [x] Re-enable ppx_errors* tests
• [x] Remove all appearances of @@@react.dom
• [x] Fix example components
• [x] Rename all functions with _ at the end (see comment below)
• [x] Think about custom elements

Out of scope

• Update dsl functions so that each element can only receive allowed props (effectively merge ppx Html module into Dsl.Html).

[jchavarri]

It seemed to me that fully qualifying all the DSL calls (e.g. React.Dom.Dsl.Html.div) would make the transition seamless. But now that I've thought it through a bit more, I realize this will only work for known HTML elements. It wouldn't work for SVG or anything else, unless we have some convention for "escaping" the HTML scope.

@glennsl bringing your comment here :) so we can continue the discussion in context of this PR. I think we have to experiment with this approach a bit. This might also guide us on the way on how to split the React.Dom.Dsl functions into separate modules.

For a first version, in this PR I am just doing very dumb transformation, and assuming user is opening the modules that are needed for the functions to type check. This might lead to issues, but I think it's easy to understand what the ppx is doing, which is also an advantage.

The only "magic" that I had to do for now is prefix the prop function with Prop to allow components to use props like html:

https://github.com/ml-in-barcelona/jsoo-react/blob/a677144d50b42b44542e025bf4854f791e84fac6/ppx/ppx.ml#L984

Also, a renaming of ref prop:

https://github.com/ml-in-barcelona/jsoo-react/blob/a677144d50b42b44542e025bf4854f791e84fac6/ppx/ppx.ml#L979

I am not sure how harmful this will be yet 😬

[glennsl]

Also, a renaming of ref prop

Ah, yeah that's a bit unfortunate. Technically it's still possible to name the prop ref, but that would shadow Stdlib.ref so that you have to qualify that every time you use it. Not including Prop in Html, and only opening Prop locally is also a way we can go. But if we want Prop to be opened non-locally I think we have to name it ref_ to avoid the shadowing.

There's also a few others that need renaming, btw. method_, open_ and type_ in Html. And begin_, end_, in_ and to_ in Svg.

[jchavarri]

There's also a few others that need renaming, btw. method_, open_ and type_ in Html. And begin_, end_, in_ and to_ in Svg.

@glennsl as i was looking into this, I thought that based on past experiences, we should try to minimize the amount of magic that happens in the ppx. So, I think it's ok if we just make the transformation always the same, and Reason jsx needs to use the real function name, e.g. ref_: https://github.com/ml-in-barcelona/jsoo-react/pull/128/commits/a4abfa6fea8b2f4691272acd79f16bc7155f83d7

What do you think?

Another thing, I saw Dom_html.float has no underscore but there's a function in OCaml stdlib with same name. What's the rule of thumb to append _ to the Dom_html function name?

[glennsl]

What do you think?

I think that's probably fine, as long as we're consistent.

Another thing, I saw Dom_html.float has no underscore but there's a function in OCaml stdlib with same name. What's the rule of thumb to append _ to the Dom_html function name?

The one's I've already suffixed have all been done just to fix compiler errors. Either because they're reserved keywords, or because I've actually encountered a problem with shadowing. The first one is simple enough, as the compiler will flat out refuse to compile if you use reserved keywords as identifiers. And the second one you could generalize to "anything that shadows the definitions in Stdlib". Then again, Stdlib is full of archaic historical artifacts and you could argue that you should use float_of_int or Float.of_int instead. But float isn't actually deprecated either.

Unless there are other counterarguments, I think we should go with the simplest rule: Avoid shadowing anything defined in Stdlib.

[jchavarri]

I'm questioning this a little bit. Is there a reason to support camelCase at all, apart from compatibility with react and rescript-react? Everything else in jsoo-land is snake_cased right?

Yeah, I agree that keeping both will be a mess in terms of documentation. I removed the camel case in https://github.com/ml-in-barcelona/jsoo-react/pull/128/commits/d3924adb125a8a7fadb8c2535486127c9b05c1fa.

Most probably the Reason "compat layer" should be isolated on its own package, where all redefinitions using uppercase could be performed. This kind of relates with st else I've been thinking, which is splitting the ppx in two, as all the @JSX attrs processing is unnecessary for OCaml syntax users, so they should not pay the costs and complexity. So maybe makes sense to package all the Reason stuff together in some jsoo-react-reason package 🤔

It kind of bothers me that all Core functions use camelCase, like useState etc. We should probably revisit them as well.

[glennsl]

Yeah I think having a separate PPX and jsoo-react-reason package is a great idea. Makes everything much cleaner. As long as they can be mixed, as I imagine some may want to try out both or migrate gradually from one to the other.

It kind of bothers me that all Core functions use camelCase, like useState etc.

Me too. If given permission I can clean those up in the near future :grin:

[jchavarri]

Me too. If given permission I can clean those up in the near future

Yes! Sounds great, this would be super appreciated 🙏