Fix react comp key type (#693)

* fix react comp key type

* add tests

* call renamed jsx binding

* add test

* add more examples

* fix key type of lowercase jsx runtime

* add more examples

* update jsx v4 spec about key type

* fix JSX v4 spec

* update changelog

* add jsx key type tests

Author: mununki

CHANGELOG.md

@@ -43,6 +43,7 @@
 - Fix several printing issues with `async` including an infinite loop https://github.com/rescript-lang/syntax/pull/680
 - Fix issue where certain JSX expressions would be formatted differenctly in compiler 10.1.0-rc.1 https://github.com/rescript-lang/syntax/issues/675
 - Fix issue where printing nested pipe discards await https://github.com/rescript-lang/syntax/issues/687
+- Fix issue where the JSX key type is not an optional string https://github.com/rescript-lang/syntax/pull/693
 
 #### :eyeglasses: Spec Compliance
 

cli/JSXV4.md

@@ -11,7 +11,7 @@ To build an entire project in V4 mode, including all its dependencies, use the n
 "jsx": { "version": 4 }
 ```
 
-> Note that JSX V4 requires the rescript compiler 10.1 or higher, and `rescript-react` version `0.11` or higher. In addition, `react` version `18.2` is required.
+> Note that JSX V4 requires the rescript compiler 10.1 or higher, and `rescript-react` version `0.11` or higher. In addition, `react` version `18.0` is required.
 
 ## Configuration And Upgrade
 
@@ -286,7 +286,11 @@ React.createElement(Comp.make, {x, y: 7, ?z})
 
 <Comp x key="7">
 // is transformed to
-React.createElement(Comp.make, Jsx.addKeyProp({x: x}, "7"))
+React.createElement(Comp.make, React.addKeyProp(~key="7", {x: x}))
+
+<Comp x key=?Some("7")>
+// is transformed to
+React.createElement(Comp.make, React.addKeyProp(~key=?Some("7"), {x: x}))
 ```
 
 ### New experimental automatic mode

cli/reactjs_jsx_v4.ml

@@ -387,49 +387,42 @@ let transformUppercaseCall3 ~config modulePath mapper jsxExprLoc callExprLoc
   | "automatic" ->
     let jsxExpr, key =
       match (!childrenArg, keyProp) with
-      | None, (_, keyExpr) :: _ ->
-        ( Exp.ident
-            {loc = Location.none; txt = Ldot (Lident "React", "jsxKeyed")},
-          [(nolabel, keyExpr)] )
+      | None, key :: _ ->
+        ( Exp.ident {loc = Location.none; txt = Ldot (Lident "React", "jsx")},
+          [key] )
       | None, [] ->
         (Exp.ident {loc = Location.none; txt = Ldot (Lident "React", "jsx")}, [])
-      | Some _, (_, keyExpr) :: _ ->
-        ( Exp.ident
-            {loc = Location.none; txt = Ldot (Lident "React", "jsxsKeyed")},
-          [(nolabel, keyExpr)] )
+      | Some _, key :: _ ->
+        ( Exp.ident {loc = Location.none; txt = Ldot (Lident "React", "jsxs")},
+          [key] )
       | Some _, [] ->
         ( Exp.ident {loc = Location.none; txt = Ldot (Lident "React", "jsxs")},
           [] )
     in
-    Exp.apply ~attrs jsxExpr ([(nolabel, makeID); (nolabel, props)] @ key)
+    Exp.apply ~attrs jsxExpr (key @ [(nolabel, makeID); (nolabel, props)])
   | _ -> (
     match (!childrenArg, keyProp) with
-    | None, (_, keyExpr) :: _ ->
+    | None, key :: _ ->
       Exp.apply ~attrs
         (Exp.ident
            {
              loc = Location.none;
              txt = Ldot (Lident "React", "createElementWithKey");
            })
-        [(nolabel, makeID); (nolabel, props); (nolabel, keyExpr)]
+        [key; (nolabel, makeID); (nolabel, props)]
     | None, [] ->
       Exp.apply ~attrs
         (Exp.ident
            {loc = Location.none; txt = Ldot (Lident "React", "createElement")})
         [(nolabel, makeID); (nolabel, props)]
-    | Some children, (_, keyExpr) :: _ ->
+    | Some children, key :: _ ->
       Exp.apply ~attrs
         (Exp.ident
            {
              loc = Location.none;
              txt = Ldot (Lident "React", "createElementVariadicWithKey");
            })
-        [
-          (nolabel, makeID);
-          (nolabel, props);
-          (nolabel, children);
-          (nolabel, keyExpr);
-        ]
+        [key; (nolabel, makeID); (nolabel, props); (nolabel, children)]
     | Some children, [] ->
       Exp.apply ~attrs
         (Exp.ident
@@ -497,23 +490,21 @@ let transformLowercaseCall3 ~config mapper jsxExprLoc callExprLoc attrs
     in
     let jsxExpr, key =
       match (!childrenArg, keyProp) with
-      | None, (_, keyExpr) :: _ ->
-        ( Exp.ident
-            {loc = Location.none; txt = Ldot (Lident "ReactDOM", "jsxKeyed")},
-          [(nolabel, keyExpr)] )
+      | None, key :: _ ->
+        ( Exp.ident {loc = Location.none; txt = Ldot (Lident "ReactDOM", "jsx")},
+          [key] )
       | None, [] ->
         ( Exp.ident {loc = Location.none; txt = Ldot (Lident "ReactDOM", "jsx")},
           [] )
-      | Some _, (_, keyExpr) :: _ ->
-        ( Exp.ident
-            {loc = Location.none; txt = Ldot (Lident "ReactDOM", "jsxsKeyed")},
-          [(nolabel, keyExpr)] )
+      | Some _, key :: _ ->
+        ( Exp.ident {loc = Location.none; txt = Ldot (Lident "ReactDOM", "jsxs")},
+          [key] )
       | Some _, [] ->
         ( Exp.ident {loc = Location.none; txt = Ldot (Lident "ReactDOM", "jsxs")},
           [] )
     in
     Exp.apply ~attrs jsxExpr
-      ([(nolabel, componentNameExpr); (nolabel, props)] @ key)
+      (key @ [(nolabel, componentNameExpr); (nolabel, props)])
   | _ ->
     let children, nonChildrenProps =
       extractChildren ~loc:jsxExprLoc callArguments

tests/ppx/react/expected/noPropsWithKey.res.txt

@@ -26,8 +26,8 @@ module V4C = {
     ReactDOM.createElement(
       React.fragment,
       [
-        React.createElementWithKey(V4CA.make, {}, "k"),
-        React.createElementWithKey(V4CB.make, {}, "k"),
+        React.createElementWithKey(~key="k", V4CA.make, {}),
+        React.createElementWithKey(~key="k", V4CB.make, {}),
       ],
     )
   let make = {
@@ -36,3 +36,39 @@ module V4C = {
     \"NoPropsWithKey$V4C"
   }
 }
+
+@@jsxConfig({version: 4, mode: "automatic"})
+
+module V4CA = {
+  type props = {}
+
+  @react.component let make = (_: props) => ReactDOM.jsx("div", {})
+  let make = {
+    let \"NoPropsWithKey$V4CA" = props => make(props)
+
+    \"NoPropsWithKey$V4CA"
+  }
+}
+
+module V4CB = {
+  type props = {}
+
+  @module("c")
+  external make: React.componentLike<props, React.element> = "component"
+}
+
+module V4C = {
+  type props = {}
+
+  @react.component
+  let make = (_: props) =>
+    React.jsxs(
+      React.jsxFragment,
+      {children: [React.jsx(~key="k", V4CA.make, {}), React.jsx(~key="k", V4CB.make, {})]},
+    )
+  let make = {
+    let \"NoPropsWithKey$V4C" = props => make(props)
+
+    \"NoPropsWithKey$V4C"
+  }
+}

tests/ppx/react/expected/optionalKeyType.res.txt

@@ -0,0 +1,77 @@
+let key = None
+
+@@jsxConfig({version: 3})
+
+let _ = React.createElement(C.make, C.makeProps(~key="k", ()))
+let _ = React.createElement(C.make, C.makeProps(~key=?Some("k"), ()))
+let _ = React.createElement(C.make, C.makeProps(~key?, ()))
+let _ = ReactDOMRe.createDOMElementVariadic("div", ~props=ReactDOMRe.domProps(~key="k", ()), [])
+let _ = ReactDOMRe.createDOMElementVariadic(
+  "div",
+  ~props=ReactDOMRe.domProps(~key=?Some("k"), ()),
+  [],
+)
+let _ = ReactDOMRe.createDOMElementVariadic("div", ~props=ReactDOMRe.domProps(~key?, ()), [])
+let _ = ReactDOMRe.createDOMElementVariadic(
+  "div",
+  ~props=ReactDOMRe.domProps(~key="k", ()),
+  [ReactDOMRe.createDOMElementVariadic("br", []), ReactDOMRe.createDOMElementVariadic("br", [])],
+)
+let _ = ReactDOMRe.createDOMElementVariadic(
+  "div",
+  ~props=ReactDOMRe.domProps(~key=?Some("k"), ()),
+  [ReactDOMRe.createDOMElementVariadic("br", []), ReactDOMRe.createDOMElementVariadic("br", [])],
+)
+let _ = ReactDOMRe.createDOMElementVariadic(
+  "div",
+  ~props=ReactDOMRe.domProps(~key?, ()),
+  [ReactDOMRe.createDOMElementVariadic("br", []), ReactDOMRe.createDOMElementVariadic("br", [])],
+)
+
+@@jsxConfig({version: 4, mode: "classic"})
+
+let _ = React.createElementWithKey(~key="k", C.make, {})
+let _ = React.createElementWithKey(~key=?Some("k"), C.make, {})
+let _ = React.createElementWithKey(~key?, C.make, {})
+let _ = ReactDOM.createDOMElementVariadic("div", ~props={key: "k"}, [])
+let _ = ReactDOM.createDOMElementVariadic("div", ~props={key: ?Some("k")}, [])
+let _ = ReactDOM.createDOMElementVariadic("div", ~props={key: ?key}, [])
+let _ = ReactDOM.createDOMElementVariadic(
+  "div",
+  ~props={key: "k"},
+  [ReactDOM.createDOMElementVariadic("br", []), ReactDOM.createDOMElementVariadic("br", [])],
+)
+let _ = ReactDOM.createDOMElementVariadic(
+  "div",
+  ~props={key: ?Some("k")},
+  [ReactDOM.createDOMElementVariadic("br", []), ReactDOM.createDOMElementVariadic("br", [])],
+)
+let _ = ReactDOM.createDOMElementVariadic(
+  "div",
+  ~props={key: ?key},
+  [ReactDOM.createDOMElementVariadic("br", []), ReactDOM.createDOMElementVariadic("br", [])],
+)
+
+@@jsxConfig({version: 4, mode: "automatic"})
+
+let _ = React.jsx(~key="k", C.make, {})
+let _ = React.jsx(~key=?Some("k"), C.make, {})
+let _ = React.jsx(~key?, C.make, {})
+let _ = ReactDOM.jsx(~key="k", "div", {})
+let _ = ReactDOM.jsx(~key=?Some("k"), "div", {})
+let _ = ReactDOM.jsx(~key?, "div", {})
+let _ = ReactDOM.jsxs(
+  ~key="k",
+  "div",
+  {children: React.array([ReactDOM.jsx("br", {}), ReactDOM.jsx("br", {})])},
+)
+let _ = ReactDOM.jsxs(
+  ~key=?Some("k"),
+  "div",
+  {children: React.array([ReactDOM.jsx("br", {}), ReactDOM.jsx("br", {})])},
+)
+let _ = ReactDOM.jsxs(
+  ~key?,
+  "div",
+  {children: React.array([ReactDOM.jsx("br", {}), ReactDOM.jsx("br", {})])},
+)

tests/ppx/react/expected/removedKeyProp.res.txt

@@ -34,14 +34,14 @@ let make = (_: props) =>
   ReactDOM.createElement(
     React.fragment,
     [
-      React.createElementWithKey(Foo.make, {x: "x", y: "y"}, "k"),
+      React.createElementWithKey(~key="k", Foo.make, {x: "x", y: "y"}),
       React.createElement(Foo.make, {x: "x", y: "y"}),
       React.createElementWithKey(
+        ~key="k",
         HasChildren.make,
         {
           children: React.createElement(Foo.make, {x: "x", y: "y"}),
         },
-        "k",
       ),
       React.createElement(
         HasChildren.make,

tests/ppx/react/expected/spreadProps.res.txt

@@ -11,6 +11,16 @@ let c1 = React.createElement(A.make, p)
 // reversed order
 let c2 = React.createElement(A.make, {...p, x: "x"})
 
+let c3 = ReactDOM.createDOMElementVariadic("div", ~props=p, [])
+
+let c4 = ReactDOM.createDOMElementVariadic("div", ~props={...p, x: "x", key: "k"}, [])
+
+let c4 = ReactDOM.createDOMElementVariadic(
+  "div",
+  ~props={...p, key: "k"},
+  [ReactDOM.createDOMElementVariadic("br", []), ReactDOM.createDOMElementVariadic("br", [])],
+)
+
 @@jsxConfig({version: 4, mode: "automatic"})
 // Error: spreadProps should be first in order than other props
 // let c0 = <A x="x" {...p} />
@@ -23,3 +33,13 @@ let c1 = React.jsx(A.make, p)
 
 // reversed order
 let c2 = React.jsx(A.make, {...p, x: "x"})
+
+let c3 = ReactDOM.jsx("div", p)
+
+let c4 = ReactDOM.jsx(~key="k", "div", {...p, x: "x"})
+
+let c5 = ReactDOM.jsxs(
+  ~key="k",
+  "div",
+  {...p, children: React.array([ReactDOM.jsx("br", {}), ReactDOM.jsx("br", {})])},
+)

tests/ppx/react/noPropsWithKey.res

@@ -10,6 +10,23 @@ module V4CB = {
   external make: unit => React.element = "component"
 }
 
+module V4C = {
+  @react.component
+  let make = () => <><V4CA key="k" /> <V4CB key="k" /></>
+}
+
+@@jsxConfig({version: 4, mode: "automatic"})
+
+module V4CA = {
+  @react.component
+  let make = () => <div />
+}
+
+module V4CB = {
+  @module("c") @react.component
+  external make: unit => React.element = "component"
+}
+
 module V4C = {
   @react.component
   let make = () => <><V4CA key="k" /> <V4CB key="k" /></>

tests/ppx/react/optionalKeyType.res

@@ -0,0 +1,37 @@
+let key = None
+
+@@jsxConfig({version:3})
+
+let _ = <C key="k" />
+let _ = <C key=?Some("k") />
+let _ = <C ?key />
+let _ = <div key="k" />
+let _ = <div key=?Some("k") />
+let _ = <div ?key />
+let _ = <div key="k"> <br /><br /> </div>
+let _ = <div key=?Some("k")> <br /><br /> </div>
+let _ = <div ?key> <br /><br /> </div>
+
+@@jsxConfig({version:4, mode: "classic"})
+
+let _ = <C key="k" />
+let _ = <C key=?Some("k") />
+let _ = <C ?key />
+let _ = <div key="k" />
+let _ = <div key=?Some("k") />
+let _ = <div ?key />
+let _ = <div key="k"> <br /><br /> </div>
+let _ = <div key=?Some("k")> <br /><br /> </div>
+let _ = <div ?key> <br /><br /> </div>
+
+@@jsxConfig({version:4, mode: "automatic"})
+
+let _ = <C key="k" />
+let _ = <C key=?Some("k") />
+let _ = <C ?key />
+let _ = <div key="k" />
+let _ = <div key=?Some("k") />
+let _ = <div ?key />
+let _ = <div key="k"> <br /><br /> </div>
+let _ = <div key=?Some("k")> <br /><br /> </div>
+let _ = <div ?key> <br /><br /> </div>

tests/ppx/react/spreadProps.res

@@ -11,6 +11,12 @@ let c1 = <A {...p} />
 // reversed order
 let c2 = <A {...p} x="x" />
 
+let c3 = <div {...p} />
+
+let c4 = <div {...p} x="x" key="k" />
+
+let c4 = <div {...p} key="k"><br /><br /></div>
+
 @@jsxConfig({version:4, mode: "automatic"})
 // Error: spreadProps should be first in order than other props
 // let c0 = <A x="x" {...p} />
@@ -23,3 +29,9 @@ let c1 = <A {...p} />
 
 // reversed order
 let c2 = <A {...p} x="x" />
+
+let c3 = <div {...p} />
+
+let c4 = <div {...p} x="x" key="k" />
+
+let c5 = <div {...p} key="k"><br /><br /></div>