WIP: React JSX PPX v4 #235

rickyvetter · 2021-01-15T16:51:12Z

This is part of the proposal discussed in https://forum.rescript-lang.org/t/rfc-rescript-react/901

There are still some changes that need to be made:

clean up JSX3/4 calling and make change to caller in compiler
move random raises to Location.raise_errorf, add tracking markers for reanalyze
clean up ReasonReact references
more thorough test cases

IwanKaramazow · 2021-01-24T13:26:28Z

src/reactjs_jsx_ppx_v4.ml

+let mkSingleVariant rf = Rtag ({loc = Location.none; txt = rf}, [], true, [])
+let mkPropVariant row_fields = Typ.variant (List.map mkSingleVariant row_fields) Closed None
+
+let allowedProps = [


Should we make this an array instead of a list?

dodomorandi

I tried to port some changed I proposed in a PR for rescript-react. As I discussed in the original comment, this is just a rough and simple check based on some comments left in the original reason-react code. Unfortunately, doing this specific check for all attributes is quite time consuming.

I added a reference for each attribute I commented, so you can easily check if I made any mistake, if you want.

dodomorandi · 2021-03-09T21:37:19Z

src/reactjs_jsx_ppx_v4.ml

+("ariaRoledescription", {jsString = Some("aria-roledescription"); typeString = mkPropConst (Lident "string")});
+("ariaAutocomplete", {jsString = Some("aria-autocomplete"); typeString = mkPropVariant ["inline"; "list"; "both"; "none"]});
+("ariaChecked", {jsString = Some("aria-checked"); typeString = mkPropVariant ["true"; "false" ;"mixed"]});
+("ariaExpanded", {jsString = Some("aria-expanded"); typeString = mkPropConst (Lident "bool")});


According to WAI-ARIA 1.1, aria-expanded can be true/false/undefined. Is it better to use mkPropVariant["true";"false";"undefined"] or an int option (which I don't know how to express in this context)?

Yeah I think these 3-state cases are where most issues will be here. When reading the spec I don't see a place where "undefined" has a behavior different from not any value at all. All true/false/undefined use undefined as default - so why provide an additional way to express when you get clean bools otherwise? I think if there are cases where the difference between not providing and providing an explicit "undefined" matters then it's a good change.

What you say is totally right. The only hypothetical case (but it is really hypothetical) is when a function receive an element and spits it out after modifying it. If ariaExpanded was set, are we able to set it to None? If this is possible, even this very niche case can be solved, therefore the undefined variant can be avoided without consequences.

dodomorandi · 2021-03-09T21:40:30Z

src/reactjs_jsx_ppx_v4.ml

+("ariaMultiselectable", {jsString = Some("aria-multiselectable"); typeString = mkPropConst (Lident "bool")});
+("ariaOrientation", {jsString = Some("aria-orientation"); typeString = mkPropVariant ["horizontal"; "vertical"; "undefined"]});
+("ariaPlaceholder", {jsString = Some("aria-placeholder"); typeString = mkPropConst (Lident "string")});
+("ariaPressed", {jsString = Some("aria-pressed"); typeString = mkPropVariant ["true"; "false" ;"mixed"]});


According to WAI-ARIA 1.1, variant "undefined" is also allowed.

Suggested change

("ariaPressed", {jsString = Some("aria-pressed"); typeString = mkPropVariant ["true"; "false" ;"mixed"]});

("ariaPressed", {jsString = Some("aria-pressed"); typeString = mkPropVariant ["true"; "false"; "mixed"; "undefined"]});

Similar as the other - does explicit "undefined" have different behavior from not providing ariaPressed at all? Much smaller downside to including here though.

Yes, in this case we already lost the bool because of the mixed variant. If we don't need to express the undefined variant as discussed before, it is just a matter of choice. ☺️

dodomorandi · 2021-03-09T21:44:58Z

src/reactjs_jsx_ppx_v4.ml

+("className", {jsString = None; typeString = mkPropConst (Lident "string")});
+("contentEditable", {jsString = None; typeString = mkPropConst (Lident "bool")});
+("contextMenu", {jsString = None; typeString = mkPropConst (Lident "string")});
+("dir", {jsString = None; typeString = mkPropConst (Lident "string")});


According to HTML 5.2, this should be an enumeration.

Suggested change

("dir", {jsString = None; typeString = mkPropConst (Lident "string")});

("dir", {jsString = None; typeString = mkPropVariant ["ltr"; "rtl"; "auto"]});

dodomorandi · 2021-03-09T21:56:19Z

src/reactjs_jsx_ppx_v4.ml

+("charSet", {jsString = None; typeString = mkPropConst (Lident "string")});
+("checked", {jsString = None; typeString = mkPropConst (Lident "bool")});
+("cite", {jsString = None; typeString = mkPropConst (Lident "string")});
+("crossOrigin", {jsString = None; typeString = mkPropConst (Lident "string")});


According to HTML 5.2, this attribute should be an enumeration. However,

The missing value default, used when the attribute is omitted, is the No CORS state.
Is it ok to just consider the two variants anonymous and use-credentials, or things should be handled in a different way?

dodomorandi · 2021-03-09T22:00:01Z

src/reactjs_jsx_ppx_v4.ml

+("defer", {jsString = None; typeString = mkPropConst (Lident "bool")});
+("disabled", {jsString = None; typeString = mkPropConst (Lident "bool")});
+("download", {jsString = None; typeString = mkPropConst (Lident "string")});
+("encType", {jsString = None; typeString = mkPropConst (Lident "string")});


According to HTML 5.2, this should be an enumerated attribute.

Suggested change

("encType", {jsString = None; typeString = mkPropConst (Lident "string")});

("encType", {jsString = None; typeString = mkPropVariant ["application/x-www-form-urlencoded"; "multipart/form-data"; "text/plain"]});

dodomorandi · 2021-03-09T22:27:01Z

src/reactjs_jsx_ppx_v4.ml

+("pattern", {jsString = None; typeString = mkPropConst (Lident "string")});
+("placeholder", {jsString = None; typeString = mkPropConst (Lident "string")});
+("poster", {jsString = None; typeString = mkPropConst (Lident "string")});
+("preload", {jsString = None; typeString = mkPropConst (Lident "string")});


According to WHATWG HTML living standard, this should be an enumerated variant.

Suggested change

("preload", {jsString = None; typeString = mkPropConst (Lident "string")});

("preload", {jsString = None; typeString = mkPropVariant ["none"; "metadata"; "auto"]});

dodomorandi · 2021-03-09T22:33:47Z

src/reactjs_jsx_ppx_v4.ml

+("rows", {jsString = None; typeString = mkPropConst (Lident "int")});
+("rowSpan", {jsString = None; typeString = mkPropConst (Lident "int")});
+("sandbox", {jsString = None; typeString = mkPropConst (Lident "string")});
+("scope", {jsString = None; typeString = mkPropConst (Lident "string")});


According to WHATWG HTML living standard, this should be an enumerated attribute.
I am not sure which is the best way to handle this, because the auto state is not set by an explicit keyword, but the missing value (and invalid values, which it does not make sense in this context) is the auto state. Should we just use mkPropVariant ["row"; "col"; "rowgroup"; "colgroup"] or something different in order to allow being explicit about the auto state?

dodomorandi · 2021-03-09T22:40:47Z

src/reactjs_jsx_ppx_v4.ml

+("sandbox", {jsString = None; typeString = mkPropConst (Lident "string")});
+("scope", {jsString = None; typeString = mkPropConst (Lident "string")});
+("scoped", {jsString = None; typeString = mkPropConst (Lident "bool")});
+("scrolling", {jsString = None; typeString = mkPropConst (Lident "string")});


According to HTML 4.01, this can assume only fixed values. Note that this is an HTML 4 only attribute.

Suggested change

("scrolling", {jsString = None; typeString = mkPropConst (Lident "string")});

("scrolling", {jsString = None; typeString = mkPropVariant ["auto"; "yes"; "no"]});

dodomorandi · 2021-03-09T22:42:26Z

src/reactjs_jsx_ppx_v4.ml

+("useMap", {jsString = None; typeString = mkPropConst (Lident "string")});
+("value", {jsString = None; typeString = mkPropConst (Lident "string")});
+("width", {jsString = None; typeString = mkPropConst (Lident "string")});
+("wrap", {jsString = None; typeString = mkPropConst (Lident "string")});


According to WHATWG HTML living standard, this should be an enumerated attribute.

Suggested change

("wrap", {jsString = None; typeString = mkPropConst (Lident "string")});

("wrap", {jsString = None; typeString = mkPropVariant ["soft"; "hard"]});

dodomorandi · 2021-03-09T22:55:53Z

src/reactjs_jsx_ppx_v4.ml

+("reversed", {jsString = None; typeString = mkPropConst (Lident "bool")});
+("rows", {jsString = None; typeString = mkPropConst (Lident "int")});
+("rowSpan", {jsString = None; typeString = mkPropConst (Lident "int")});
+("sandbox", {jsString = None; typeString = mkPropConst (Lident "string")});


According to WHATWG HTML living standard, this should be

an unordered set of unique space-separated tokens

which means that, in theory, we should take a pseudo-list of a variant type that can be transformed into a string. I don't know if this can be expressed, and it's not the end of the world if it cannot be done -- just worth mentioning it.

cristianoc · 2022-06-17T11:14:35Z

Continued here, in spirit: #547

rickyvetter added 5 commits January 15, 2021 11:29

WIP: jsx ppx v4

3d81e0a

lower to jsx and rmeove ReactDOMRe

23b99f1

aria variants

433e705

use data-foo instead of datafoo for lowercase jsx

6cf1be8

potential testing strategy

e9eeabf

IwanKaramazow reviewed Jan 24, 2021

View reviewed changes

ryyppy mentioned this pull request Jan 27, 2021

v1.0 Release Tasks rescript-lang/rescript-react#4

Open

4 tasks

rickyvetter mentioned this pull request Mar 9, 2021

Improve ReadDOM properties using polyvariants rescript-lang/rescript-react#12

Closed

dodomorandi suggested changes Mar 9, 2021

View reviewed changes

davesnx mentioned this pull request Nov 25, 2021

Improve type-safety of string based props ml-in-barcelona/jsoo-react#69

Open

ryyppy added this to the v10 milestone May 3, 2022

ryyppy modified the milestones: v10 ("The first community powered release"), v10.1 May 17, 2022

cristianoc mentioned this pull request Jun 6, 2022

JSX V4 WIP #517

Closed

cristianoc closed this Jun 17, 2022

davesnx mentioned this pull request Nov 5, 2023

Improve type-safety of string-based props davesnx/html_of_jsx#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: React JSX PPX v4 #235

WIP: React JSX PPX v4 #235

rickyvetter commented Jan 15, 2021

IwanKaramazow Jan 24, 2021

dodomorandi left a comment

dodomorandi Mar 9, 2021

rickyvetter Mar 12, 2021

dodomorandi Mar 12, 2021

dodomorandi Mar 9, 2021

rickyvetter Mar 12, 2021

dodomorandi Mar 12, 2021

dodomorandi Mar 9, 2021

dodomorandi Mar 9, 2021

dodomorandi Mar 9, 2021

dodomorandi Mar 9, 2021

dodomorandi Mar 9, 2021

dodomorandi Mar 9, 2021

dodomorandi Mar 9, 2021

dodomorandi Mar 9, 2021

cristianoc commented Jun 17, 2022

	("ariaPressed", {jsString = Some("aria-pressed"); typeString = mkPropVariant ["true"; "false" ;"mixed"]});
	("ariaPressed", {jsString = Some("aria-pressed"); typeString = mkPropVariant ["true"; "false"; "mixed"; "undefined"]});

	("dir", {jsString = None; typeString = mkPropConst (Lident "string")});
	("dir", {jsString = None; typeString = mkPropVariant ["ltr"; "rtl"; "auto"]});

	("encType", {jsString = None; typeString = mkPropConst (Lident "string")});
	("encType", {jsString = None; typeString = mkPropVariant ["application/x-www-form-urlencoded"; "multipart/form-data"; "text/plain"]});

	("preload", {jsString = None; typeString = mkPropConst (Lident "string")});
	("preload", {jsString = None; typeString = mkPropVariant ["none"; "metadata"; "auto"]});

	("scrolling", {jsString = None; typeString = mkPropConst (Lident "string")});
	("scrolling", {jsString = None; typeString = mkPropVariant ["auto"; "yes"; "no"]});

	("wrap", {jsString = None; typeString = mkPropConst (Lident "string")});
	("wrap", {jsString = None; typeString = mkPropVariant ["soft"; "hard"]});

WIP: React JSX PPX v4 #235

WIP: React JSX PPX v4 #235

Conversation

rickyvetter commented Jan 15, 2021

Choose a reason for hiding this comment

dodomorandi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc commented Jun 17, 2022