ml-in-barcelona / jsoo-react

js_of_ocaml bindings for ReactJS. Based on ReasonReact.
https://ml-in-barcelona.github.io/jsoo-react
MIT License
138 stars 19 forks source link

New dsl, step 3/3: migrate tag-specific prop types from Html to Dom_html #136

Open jchavarri opened 2 years ago

jchavarri commented 2 years ago

After original api implementation in https://github.com/ml-in-barcelona/jsoo-react/pull/119 and update of ppx to support it in https://github.com/ml-in-barcelona/jsoo-react/pull/128, the last step of this new api consists on migrating the "specs" implemented with types in ppx Html module to the Prop module in Dom_html. The goal being to make impossible to pass wrong attributes to e.g. a div.

I am not sure about the best way to implement this, probably could use an approach similar to bs-css, with polyvariants + subtyping to reflect inheritance or composition based on smaller subsets of props, like here.

glennsl commented 2 years ago

This sounds very complicated, and I'm not really sure it's worth it. I can't remember a single instance of having used the wrong attributes for a tag, which might also be because the consequence of doing so is entirely benign. In almost all cases it would just not do anything. You might get a warning in the browser console, and HTML validation would fail, but that's about it.

There is one feature that I think would make it worthwhile, however: The ability to auto-complete just the valid props for a tag. Unfortunately I have no idea how to even approach a solution for that problem.

glennsl commented 2 years ago

I also think it was actually me who came up with the scheme of using polyvariants to type CSS. At least I had a proper go at it with bs-typed-css quite a few years ago, and came to two conclusions from that: 1) it's very hard, and 2) there's very little benefit. Today I just use strings as values, and have very few issues with that approach.

It is a very fun challenge to try to come up with a scheme to make such a complicated system well-typed, however. So please don't let this discourage you from experimenting with it. I'm just not convinced this is all that valuable for production code.

jchavarri commented 2 years ago

Yeah I think these points are worth considering. What you mention makes me think about tyxml, which also left me (as a user) with similar feeling: the time spent trying to debug type errors was > time saved by elements being created with wrong attributes, just because the latter happens so rarely.

In general, considering how few resources jsoo-react has :) I tend to pick the simplicity path over the complexity one, unless the complexity is absolutely needed.

On the other hand, to make full argument, the constraints (I mean, the set of attributes that each element allows) are very unlikely to change over time, and also, regarding how complex the implementation is, users will not perceive this complexity, because the api design does not need to change from their perspective.

@davesnx what do you think?

davesnx commented 2 years ago

I fully agree, I personally prefer to keep the API very unsafe in those cases. bs-css makes it look easy for simple cases, but for using more advanced CSS features is totally a nightmare, is one of the reasons that I started styled-ppx in the first place!