From 4d1f2176b19df5176e9227139c430564837a3d96 Mon Sep 17 00:00:00 2001 From: Javier Chavarri Date: Thu, 15 Aug 2024 13:34:36 +0000 Subject: [PATCH 1/4] fix multi-child fragment --- .ocamlformat | 2 +- ppx/reason_react_ppx.ml | 69 ++++++++++++++++++++++++++++++++++---- ppx/test/component.t/run.t | 2 +- ppx/test/fragment.t/run.t | 6 ++-- 4 files changed, 68 insertions(+), 11 deletions(-) diff --git a/.ocamlformat b/.ocamlformat index ed7d4b31d..8a3b53e71 100644 --- a/.ocamlformat +++ b/.ocamlformat @@ -1 +1 @@ -version = 0.26.0 +version = 0.26.2 diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml index cbceb3437..5003162d6 100644 --- 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,27 @@ 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 + ( Pexp_construct ({ txt = Lident "[]"; loc }, None) + | Pexp_construct ( { txt = Lident "::"; loc }, - Some { pexp_desc = Pexp_tuple _; _ } ) - | Pexp_construct ({ txt = Lident "[]"; loc }, None) ); + Some + { + pexp_desc = + Pexp_tuple + [ + _; + { + pexp_desc = + Pexp_construct ({ txt = Lident "[]"; _ }, None); + _; + }; + ]; + _; + } ) ); pexp_attributes; _; } as listItems -> ( @@ -1418,6 +1438,43 @@ let jsxMapper = (* throw away the [@JSX] attribute and keep the others, if any *) Binding.React.jsxFragment ~loc ~attrs:nonJSXAttributes (Binding.React.array ~loc childrenExpr)) + (* Fragment with two or more children: <> foo bar *) + | { + pexp_desc = + Pexp_construct + ( { txt = Lident "::"; loc }, + Some + { + pexp_desc = + Pexp_tuple + [ + _firstElement; + { + pexp_desc = + Pexp_construct ({ txt = Lident "::"; _ }, Some _); + _; + }; + ]; + _; + } ); + pexp_attributes; + _; + } as listItems -> ( + let jsxAttribute, nonJSXAttributes = + List.partition + (fun { attr_name = attribute; _ } -> attribute.txt = "JSX") + pexp_attributes + in + match (jsxAttribute, nonJSXAttributes) with + (* no JSX attribute *) + | [], _ -> super#expression ctxt expr + | _, nonJSXAttributes -> + let childrenExpr = + transformChildrenIfList ~loc ~ctxt ~mapper:self listItems + in + (* throw away the [@JSX] attribute and keep the others, if any *) + Binding.React.jsxsFragment ~loc ~attrs:nonJSXAttributes + (Binding.React.array ~loc childrenExpr)) (* 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 index 29ff6dd64..aa9583ce1 100644 --- 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 index 62789e138..be148546c 100644 --- a/ppx/test/fragment.t/run.t +++ b/ppx/test/fragment.t/run.t @@ -11,7 +11,7 @@ ([@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|]), @@ -19,13 +19,13 @@ ), ); 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|]), From e5eeb3f23fd08fe028f7220f87caa5b86d3161c2 Mon Sep 17 00:00:00 2001 From: Javier Chavarri Date: Thu, 15 Aug 2024 14:11:28 +0000 Subject: [PATCH 2/4] revert ocamlformat change --- .ocamlformat | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.ocamlformat b/.ocamlformat index 8a3b53e71..ed7d4b31d 100644 --- a/.ocamlformat +++ b/.ocamlformat @@ -1 +1 @@ -version = 0.26.2 +version = 0.26.0 From 5f83a0d29bc9f3457eb8c00ba9e281b1f744f007 Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Thu, 15 Aug 2024 20:13:41 -0700 Subject: [PATCH 3/4] refactor: share more code --- ppx/reason_react_ppx.ml | 63 ++++++++++------------------------------- 1 file changed, 15 insertions(+), 48 deletions(-) diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml index 5003162d6..9646da10c 100644 --- a/ppx/reason_react_ppx.ml +++ b/ppx/reason_react_ppx.ml @@ -1402,24 +1402,7 @@ let jsxMapper = [@JSX][foo] This will match either <> or <> foo *) | { - pexp_desc = - ( Pexp_construct ({ txt = Lident "[]"; loc }, None) - | Pexp_construct - ( { txt = Lident "::"; loc }, - Some - { - pexp_desc = - Pexp_tuple - [ - _; - { - pexp_desc = - Pexp_construct ({ txt = Lident "[]"; _ }, None); - _; - }; - ]; - _; - } ) ); + pexp_desc = Pexp_construct ({ txt = Lident ("[]" | "::"); loc }, lst); pexp_attributes; _; } as listItems -> ( @@ -1431,50 +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)) - (* Fragment with two or more children: <> foo bar *) - | { - pexp_desc = - Pexp_construct - ( { txt = Lident "::"; loc }, - Some + match lst with + | None + | Some { pexp_desc = Pexp_tuple [ - _firstElement; + _; { pexp_desc = - Pexp_construct ({ txt = Lident "::"; _ }, Some _); + Pexp_construct ({ txt = Lident "[]"; _ }, None); _; }; ]; _; - } ); - pexp_attributes; - _; - } as listItems -> ( - let jsxAttribute, nonJSXAttributes = - List.partition - (fun { attr_name = attribute; _ } -> attribute.txt = "JSX") - pexp_attributes - in - match (jsxAttribute, nonJSXAttributes) with - (* no JSX attribute *) - | [], _ -> super#expression ctxt expr - | _, nonJSXAttributes -> - let childrenExpr = - transformChildrenIfList ~loc ~ctxt ~mapper:self listItems - in - (* throw away the [@JSX] attribute and keep the others, if any *) - Binding.React.jsxsFragment ~loc ~attrs:nonJSXAttributes - (Binding.React.array ~loc childrenExpr)) + } -> + 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] From ffb417dca65b7ad2f27daf69bc201b120ddb2fd1 Mon Sep 17 00:00:00 2001 From: Javier Chavarri Date: Fri, 16 Aug 2024 06:21:02 +0000 Subject: [PATCH 4/4] +changelog --- CHANGES.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index d1dd3df05..5e633fd35 100644 --- 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