Fix multi-child fragment (#852)

* fix multi-child fragment * revert ocamlformat change * refactor: share more code * +changelog --------- Co-authored-by: Antonio Nuno Monteiro <[email protected]>
reasonml · Aug 16, 2024 · 8d6b326 · 8d6b326
1 parent b60bd36
commit 8d6b326
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 16 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -3,7 +3,8 @@
 * Convert `ReasonReactErrorBoundary` to Reason instead of `%raw` JS. This has
   the benefit of skipping a hardcoded `require('react')` call (@anmonteiro in
   [#839](https://github.com/reasonml/reason-react/pull/839))
-
+* Fix: Remove "unique `key` prop" warnings from multi-child fragment elements
+  (@jchavarri in https://github.com/reasonml/reason-react/pull/852)
 
 # 0.14.1
 

diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml
@@ -72,19 +72,25 @@ module Binding = struct
         ( { loc; txt = Ldot (Lident "React", "componentLike") },
           [ props; return ] )
 
-    let jsxFragment ~loc ~attrs children =
+    let makeJsxFragment api ~loc ~attrs children =
       let fragment =
         Builder.pexp_ident ~loc
           { loc; txt = Ldot (Lident "React", "jsxFragment") }
       in
       Builder.pexp_apply ~loc ~attrs
-        (Builder.pexp_ident ~loc { loc; txt = Ldot (Lident "React", "jsx") })
+        (Builder.pexp_ident ~loc { loc; txt = Ldot (Lident "React", api) })
         [
           (nolabel, fragment);
           ( nolabel,
             ReactDOM.domProps ~applyLoc:loc ~loc
               [ (labelled "children", children); (nolabel, Builder.unit) ] );
         ]
+
+    let jsxFragment ~loc ~attrs children =
+      makeJsxFragment "jsx" ~loc ~attrs children
+
+    let jsxsFragment ~loc ~attrs children =
+      makeJsxFragment "jsxs" ~loc ~attrs children
   end
 end
 
@@ -1393,13 +1399,10 @@ let jsxMapper =
               transformJsxCall ~ctxt parentExpLoc self callExpression
                 callArguments nonJSXAttributes)
       (* is it a list with jsx attribute? Reason <>foo</> desugars to
-         [@JSX][foo]*)
+         [@JSX][foo]
+         This will match either <> </> or <> foo </> *)
       | {
-          pexp_desc =
-            ( Pexp_construct
-                ( { txt = Lident "::"; loc },
-                  Some { pexp_desc = Pexp_tuple _; _ } )
-            | Pexp_construct ({ txt = Lident "[]"; loc }, None) );
+          pexp_desc = Pexp_construct ({ txt = Lident ("[]" | "::"); loc }, lst);
           pexp_attributes;
           _;
         } as listItems -> (
@@ -1411,13 +1414,34 @@ let jsxMapper =
           match (jsxAttribute, nonJSXAttributes) with
           (* no JSX attribute *)
           | [], _ -> super#expression ctxt expr
-          | _, nonJSXAttributes ->
+          | _, nonJSXAttributes -> (
               let childrenExpr =
                 transformChildrenIfList ~loc ~ctxt ~mapper:self listItems
               in
               (* throw away the [@JSX] attribute and keep the others, if any *)
-              Binding.React.jsxFragment ~loc ~attrs:nonJSXAttributes
-                (Binding.React.array ~loc childrenExpr))
+              match lst with
+              | None
+              | Some
+                  {
+                    pexp_desc =
+                      Pexp_tuple
+                        [
+                          _;
+                          {
+                            pexp_desc =
+                              Pexp_construct ({ txt = Lident "[]"; _ }, None);
+                            _;
+                          };
+                        ];
+                    _;
+                  } ->
+                  Binding.React.jsxFragment ~loc ~attrs:nonJSXAttributes
+                    (Binding.React.array ~loc childrenExpr)
+              | Some { pexp_desc = Pexp_tuple (_ :: _); _ } ->
+                  (* Fragment with two or more children: <> foo bar </> *)
+                  Binding.React.jsxsFragment ~loc ~attrs:nonJSXAttributes
+                    (Binding.React.array ~loc childrenExpr)
+              | _ -> assert false))
       (* Delegate to the default mapper, a deep identity traversal *)
       | e -> super#expression ctxt e
     [@@raises Invalid_argument]

diff --git a/ppx/test/component.t/run.t b/ppx/test/component.t/run.t
@@ -27,7 +27,7 @@ We need to output ML syntax here, otherwise refmt could not parse it.
           ""[@@mel.obj ]
       let make =
         ((fun ?(name= "") ->
-            React.jsx React.jsxFragment
+            React.jsxs React.jsxFragment
               (((ReactDOM.domProps)[@merlin.hide ])
                  ~children:(React.array
                               [|(ReactDOM.jsx "div"

diff --git a/ppx/test/fragment.t/run.t b/ppx/test/fragment.t/run.t
@@ -11,21 +11,21 @@
       ([@merlin.hide] ReactDOM.domProps)(~children=React.array([|bar|]), ()),
     );
   let poly_children_fragment = (foo, bar) =>
-    React.jsx(
+    React.jsxs(
       React.jsxFragment,
       ([@merlin.hide] ReactDOM.domProps)(
         ~children=React.array([|foo, bar|]),
         (),
       ),
     );
   let nested_fragment = (foo, bar, baz) =>
-    React.jsx(
+    React.jsxs(
       React.jsxFragment,
       ([@merlin.hide] ReactDOM.domProps)(
         ~children=
           React.array([|
             foo,
-            React.jsx(
+            React.jsxs(
               React.jsxFragment,
               ([@merlin.hide] ReactDOM.domProps)(
                 ~children=React.array([|bar, baz|]),