From 06402e878deab270c48dbfb608e0c3edfe77b0a4 Mon Sep 17 00:00:00 2001 From: Javier Chavarri Date: Tue, 18 Jul 2023 17:35:56 +0000 Subject: [PATCH 01/13] ppx: add test for inference regression in keys --- test/React__test.re | 153 ++++++++++++++++++++++++++++---------------- 1 file changed, 98 insertions(+), 55 deletions(-) diff --git a/test/React__test.re b/test/React__test.re index fed88b4a3..f6d2d59b5 100644 --- a/test/React__test.re +++ b/test/React__test.re @@ -172,7 +172,8 @@ describe("React", () => { container ->DOM.findBySelectorAndTextContent("div", "Hello world!") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); }); test("can render null elements", () => { @@ -184,7 +185,8 @@ describe("React", () => { container ->DOM.findBySelectorAndPartialTextContent("div", "") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); }); test("can render string elements", () => { @@ -198,7 +200,8 @@ describe("React", () => { container ->DOM.findBySelectorAndPartialTextContent("div", "Hello") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); }); test("can render int elements", () => { @@ -210,7 +213,8 @@ describe("React", () => { container ->DOM.findBySelectorAndPartialTextContent("div", "12345") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); }); test("can render float elements", () => { @@ -222,7 +226,8 @@ describe("React", () => { container ->DOM.findBySelectorAndPartialTextContent("div", "12.345") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); }); test("can render array of elements", () => { @@ -237,19 +242,22 @@ describe("React", () => { container ->DOM.findBySelectorAndPartialTextContent("div", "1") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); expect( container ->DOM.findBySelectorAndPartialTextContent("div", "2") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); expect( container ->DOM.findBySelectorAndPartialTextContent("div", "3") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); }); test("can clone an element", () => { @@ -272,7 +280,8 @@ describe("React", () => { "Hello", ) ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); }); test("can render react components", () => { @@ -284,7 +293,8 @@ describe("React", () => { container ->DOM.findBySelectorAndTextContent("button", "0") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); let button = container->DOM.findBySelector("button"); @@ -299,13 +309,15 @@ describe("React", () => { container ->DOM.findBySelectorAndTextContent("button", "0") ->Option.isSome, - )->toBe(false); + ) + ->toBe(false); expect( container ->DOM.findBySelectorAndTextContent("button", "1") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); }); test("can render react components with reducers", () => { @@ -317,7 +329,8 @@ describe("React", () => { container ->DOM.findBySelectorAndTextContent(".value", "0") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); let button = container->DOM.findBySelectorAndPartialTextContent( @@ -336,13 +349,15 @@ describe("React", () => { container ->DOM.findBySelectorAndTextContent(".value", "0") ->Option.isSome, - )->toBe(false); + ) + ->toBe(false); expect( container ->DOM.findBySelectorAndTextContent(".value", "1") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); let button = container->DOM.findBySelectorAndPartialTextContent( @@ -361,13 +376,15 @@ describe("React", () => { container ->DOM.findBySelectorAndTextContent(".value", "0") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); expect( container ->DOM.findBySelectorAndTextContent(".value", "1") ->Option.isSome, - )->toBe(false); + ) + ->toBe(false); }); test("can render react components with reducers (map state)", () => { @@ -381,7 +398,8 @@ describe("React", () => { container ->DOM.findBySelectorAndTextContent(".value", "1") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); let button = container->DOM.findBySelectorAndPartialTextContent( @@ -400,13 +418,15 @@ describe("React", () => { container ->DOM.findBySelectorAndTextContent(".value", "1") ->Option.isSome, - )->toBe(false); + ) + ->toBe(false); expect( container ->DOM.findBySelectorAndTextContent(".value", "2") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); let button = container->DOM.findBySelectorAndPartialTextContent( @@ -425,13 +445,15 @@ describe("React", () => { container ->DOM.findBySelectorAndTextContent(".value", "1") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); expect( container ->DOM.findBySelectorAndTextContent(".value", "2") ->Option.isSome, - )->toBe(false); + ) + ->toBe(false); }); test("can render react components with effects", () => { @@ -457,10 +479,7 @@ describe("React", () => { ) }); - expect(callback->Mock.getMock->Mock.calls)->toEqual([| - [|0|], - [|1|], - |]); + expect(callback->Mock.getMock->Mock.calls)->toEqual([|[|0|], [|1|]|]); }); test("can render react components with layout effects", () => { @@ -486,10 +505,7 @@ describe("React", () => { ) }); - expect(callback->Mock.getMock->Mock.calls)->toEqual([| - [|0|], - [|1|], - |]); + expect(callback->Mock.getMock->Mock.calls)->toEqual([|[|0|], [|1|]|]); }); test("can work with React refs", () => { @@ -510,9 +526,8 @@ describe("React", () => { ReactDOM.render(, container) }); - expect(myRef.contents->Option.map(item => item.current))->toEqual( - Some(2), - ); + expect(myRef.contents->Option.map(item => item.current)) + ->toEqual(Some(2)); }); test("Children", () => { @@ -533,19 +548,22 @@ describe("React", () => { container ->DOM.findBySelectorAndPartialTextContent("div[data-index='0']", "1") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); expect( container ->DOM.findBySelectorAndPartialTextContent("div[data-index='1']", "2") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); expect( container ->DOM.findBySelectorAndPartialTextContent("div[data-index='2']", "3") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); }); test("Context", () => { @@ -564,7 +582,8 @@ describe("React", () => { container ->DOM.findBySelectorAndPartialTextContent("div", "10") ->Option.isSome, - )->toBe(true); + ) + ->toBe(true); }); test("Events", () => { @@ -609,30 +628,54 @@ describe("React", () => { ) ->toBe(true); }); - - /* test("ErrorBoundary", () => { + + test("Type inference with keys", () => { let container = getContainer(container); + module Author = { + type t = { + name: string, + imageUrl: string, + }; + }; + + let render = author => + + + ; + act(() => { ReactDOM.render( - { - expect( - info.componentStack->Js.String2.includes("ComponentThatThrows"), - ). - toBe(true); - "An error occured"->React.string ; - }}> - - , + render({name: "Joe", imageUrl: "https://foo.png"}), container, ) }); - expect( - container - ->DOM.findBySelectorAndTextContent("strong", "An error occured") - ->Option.isSome, - )->toBe(true); - }); */ + expect(container->DOM.findBySelector("img")->Option.isSome)->toBe(true); + }); + /* test("ErrorBoundary", () => { + let container = getContainer(container); + + act(() => { + ReactDOM.render( + { + expect( + info.componentStack->Js.String2.includes("ComponentThatThrows"), + ). + toBe(true); + "An error occured"->React.string ; + }}> + + , + container, + ) + }); + + expect( + container + ->DOM.findBySelectorAndTextContent("strong", "An error occured") + ->Option.isSome, + )->toBe(true); + }); */ }); From 84254679f31f84aae7638cdec5836dc31708254f Mon Sep 17 00:00:00 2001 From: Javier Chavarri Date: Wed, 19 Jul 2023 07:59:40 +0000 Subject: [PATCH 02/13] reactdom, ppx: pass key before children --- ppx/src/reactjs_jsx_ppx.ml | 24 ++++++++++++------------ ppx/test/output.expected | 7 +++---- src/React.re | 2 ++ src/ReactDOM.re | 4 ++++ 4 files changed, 21 insertions(+), 16 deletions(-) diff --git a/ppx/src/reactjs_jsx_ppx.ml b/ppx/src/reactjs_jsx_ppx.ml index 206f8982a..73e53bd95 100644 --- a/ppx/src/reactjs_jsx_ppx.ml +++ b/ppx/src/reactjs_jsx_ppx.ml @@ -457,7 +457,6 @@ let makeExternalDecl fnName loc namedArgListWithKeyAndRef namedTypeList = (* TODO: some line number might still be wrong *) let jsxMapper = - let unit = Exp.construct { txt = Lident "()"; loc = Location.none } None in let transformUppercaseCall3 ~caller modulePath ~ctxt mapper loc attrs _ callArguments = let children, argsWithLabels = extractChildren callArguments in @@ -502,15 +501,15 @@ let jsxMapper = let props = Exp.apply ~loc (Exp.ident ~loc { loc; txt = propsIdent }) propsArg in - let key_args = + let args = + let elementArg = (nolabel, Exp.ident ~loc { txt = ident; loc }) in + let propsArg = (nolabel, props) in match key with | Some (label, key) -> - [ (label, mapper#expression ctxt key); (nolabel, unit) ] - | None -> [] + [ elementArg; (label, mapper#expression ctxt key); propsArg ] + | None -> [ elementArg; propsArg ] in - Exp.apply ~loc ~attrs jsxExpr - ([ (nolabel, Exp.ident ~loc { txt = ident; loc }); (nolabel, props) ] - @ key_args) + Exp.apply ~loc ~attrs jsxExpr args in let transformLowercaseCall3 ~ctxt parentExpLoc mapper loc attrs callArguments @@ -539,14 +538,15 @@ let jsxMapper = |> List.map (fun (label, expression) -> (label, mapper#expression ctxt expression))) in - let key_args = + let args = + let elementArg = (nolabel, componentNameExpr) in + let propsArg = (nolabel, propsCall) in match key with | Some (label, key) -> - [ (label, mapper#expression ctxt key); (nolabel, unit) ] - | None -> [] + [ elementArg; (label, mapper#expression ctxt key); propsArg ] + | None -> [ elementArg; propsArg ] in - ( jsxExpr, - [ (nolabel, componentNameExpr); (nolabel, propsCall) ] @ key_args ) + (jsxExpr, args) in Exp.apply ~loc:parentExpLoc ~attrs jsxExpr args in diff --git a/ppx/test/output.expected b/ppx/test/output.expected index ff730bf2e..92af43d2b 100644 --- a/ppx/test/output.expected +++ b/ppx/test/output.expected @@ -58,6 +58,8 @@ let lower_children_nested = (fun e -> ReactDOM.jsxKeyed "li" + ~key:( + e.path) (((ReactDOM.domProps) [@merlin.hide ]) @@ -79,10 +81,7 @@ let lower_children_nested = event; ReactRouter.push e.path) - ())) ()) - ~key:( - e.path) - ()))) + ())) ())))) |> React.list) ())) ~className:"menu" ()))|]) diff --git a/src/React.re b/src/React.re index 5f70d34f9..ce420ac11 100644 --- a/src/React.re +++ b/src/React.re @@ -32,6 +32,7 @@ external createElementVariadic: external jsxKeyed: (component('props), 'props, ~key: string=?, unit) => element = "jsx"; +let jsxKeyed = (comp, ~key=?, props) => jsxKeyed(comp, props, ~key?, ()); [@bs.module "react/jsx-runtime"] external jsx: (component('props), 'props) => element = "jsx"; @@ -43,6 +44,7 @@ external jsxs: (component('props), 'props) => element = "jsxs"; external jsxsKeyed: (component('props), 'props, ~key: string=?, unit) => element = "jsxs"; +let jsxsKeyed = (comp, ~key=?, props) => jsxsKeyed(comp, props, ~key?, ()); [@bs.module "react/jsx-runtime"] external jsxFragment: 'element = "Fragment"; diff --git a/src/ReactDOM.re b/src/ReactDOM.re index c79d5c93b..a113ead22 100644 --- a/src/ReactDOM.re +++ b/src/ReactDOM.re @@ -2138,6 +2138,8 @@ external createDOMElementVariadic: [@bs.module "react/jsx-runtime"] external jsxKeyed: (string, domProps, ~key: string=?, unit) => React.element = "jsx"; +let jsxKeyed = (element, ~key=?, props) => + jsxKeyed(element, props, ~key?, ()); [@bs.module "react/jsx-runtime"] external jsx: (string, domProps) => React.element = "jsx"; @@ -2148,3 +2150,5 @@ external jsxs: (string, domProps) => React.element = "jsxs"; [@bs.module "react/jsx-runtime"] external jsxsKeyed: (string, domProps, ~key: string=?, unit) => React.element = "jsxs"; +let jsxsKeyed = (element, ~key=?, props) => + jsxsKeyed(element, props, ~key?, ()); From 6f497a647dd3cb5d2308b78f6ae0e2d64641f703 Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Wed, 19 Jul 2023 13:39:02 -0700 Subject: [PATCH 03/13] Revert "reactdom, ppx: pass key before children" This reverts commit 84254679f31f84aae7638cdec5836dc31708254f. --- ppx/src/reactjs_jsx_ppx.ml | 24 ++++++++++++------------ ppx/test/output.expected | 7 ++++--- src/React.re | 2 -- src/ReactDOM.re | 4 ---- 4 files changed, 16 insertions(+), 21 deletions(-) diff --git a/ppx/src/reactjs_jsx_ppx.ml b/ppx/src/reactjs_jsx_ppx.ml index 73e53bd95..206f8982a 100644 --- a/ppx/src/reactjs_jsx_ppx.ml +++ b/ppx/src/reactjs_jsx_ppx.ml @@ -457,6 +457,7 @@ let makeExternalDecl fnName loc namedArgListWithKeyAndRef namedTypeList = (* TODO: some line number might still be wrong *) let jsxMapper = + let unit = Exp.construct { txt = Lident "()"; loc = Location.none } None in let transformUppercaseCall3 ~caller modulePath ~ctxt mapper loc attrs _ callArguments = let children, argsWithLabels = extractChildren callArguments in @@ -501,15 +502,15 @@ let jsxMapper = let props = Exp.apply ~loc (Exp.ident ~loc { loc; txt = propsIdent }) propsArg in - let args = - let elementArg = (nolabel, Exp.ident ~loc { txt = ident; loc }) in - let propsArg = (nolabel, props) in + let key_args = match key with | Some (label, key) -> - [ elementArg; (label, mapper#expression ctxt key); propsArg ] - | None -> [ elementArg; propsArg ] + [ (label, mapper#expression ctxt key); (nolabel, unit) ] + | None -> [] in - Exp.apply ~loc ~attrs jsxExpr args + Exp.apply ~loc ~attrs jsxExpr + ([ (nolabel, Exp.ident ~loc { txt = ident; loc }); (nolabel, props) ] + @ key_args) in let transformLowercaseCall3 ~ctxt parentExpLoc mapper loc attrs callArguments @@ -538,15 +539,14 @@ let jsxMapper = |> List.map (fun (label, expression) -> (label, mapper#expression ctxt expression))) in - let args = - let elementArg = (nolabel, componentNameExpr) in - let propsArg = (nolabel, propsCall) in + let key_args = match key with | Some (label, key) -> - [ elementArg; (label, mapper#expression ctxt key); propsArg ] - | None -> [ elementArg; propsArg ] + [ (label, mapper#expression ctxt key); (nolabel, unit) ] + | None -> [] in - (jsxExpr, args) + ( jsxExpr, + [ (nolabel, componentNameExpr); (nolabel, propsCall) ] @ key_args ) in Exp.apply ~loc:parentExpLoc ~attrs jsxExpr args in diff --git a/ppx/test/output.expected b/ppx/test/output.expected index 92af43d2b..ff730bf2e 100644 --- a/ppx/test/output.expected +++ b/ppx/test/output.expected @@ -58,8 +58,6 @@ let lower_children_nested = (fun e -> ReactDOM.jsxKeyed "li" - ~key:( - e.path) (((ReactDOM.domProps) [@merlin.hide ]) @@ -81,7 +79,10 @@ let lower_children_nested = event; ReactRouter.push e.path) - ())) ())))) + ())) ()) + ~key:( + e.path) + ()))) |> React.list) ())) ~className:"menu" ()))|]) diff --git a/src/React.re b/src/React.re index ce420ac11..5f70d34f9 100644 --- a/src/React.re +++ b/src/React.re @@ -32,7 +32,6 @@ external createElementVariadic: external jsxKeyed: (component('props), 'props, ~key: string=?, unit) => element = "jsx"; -let jsxKeyed = (comp, ~key=?, props) => jsxKeyed(comp, props, ~key?, ()); [@bs.module "react/jsx-runtime"] external jsx: (component('props), 'props) => element = "jsx"; @@ -44,7 +43,6 @@ external jsxs: (component('props), 'props) => element = "jsxs"; external jsxsKeyed: (component('props), 'props, ~key: string=?, unit) => element = "jsxs"; -let jsxsKeyed = (comp, ~key=?, props) => jsxsKeyed(comp, props, ~key?, ()); [@bs.module "react/jsx-runtime"] external jsxFragment: 'element = "Fragment"; diff --git a/src/ReactDOM.re b/src/ReactDOM.re index a113ead22..c79d5c93b 100644 --- a/src/ReactDOM.re +++ b/src/ReactDOM.re @@ -2138,8 +2138,6 @@ external createDOMElementVariadic: [@bs.module "react/jsx-runtime"] external jsxKeyed: (string, domProps, ~key: string=?, unit) => React.element = "jsx"; -let jsxKeyed = (element, ~key=?, props) => - jsxKeyed(element, props, ~key?, ()); [@bs.module "react/jsx-runtime"] external jsx: (string, domProps) => React.element = "jsx"; @@ -2150,5 +2148,3 @@ external jsxs: (string, domProps) => React.element = "jsxs"; [@bs.module "react/jsx-runtime"] external jsxsKeyed: (string, domProps, ~key: string=?, unit) => React.element = "jsxs"; -let jsxsKeyed = (element, ~key=?, props) => - jsxsKeyed(element, props, ~key?, ()); From c1841fb47c57f121cd8e2eefe6c40a371f5565c9 Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Wed, 19 Jul 2023 14:22:25 -0700 Subject: [PATCH 04/13] fix: use a let binding to match expected inference --- ppx/src/reactjs_jsx_ppx.ml | 98 +++++++++++++++++++++++--------------- 1 file changed, 60 insertions(+), 38 deletions(-) diff --git a/ppx/src/reactjs_jsx_ppx.ml b/ppx/src/reactjs_jsx_ppx.ml index 206f8982a..94027fd02 100644 --- a/ppx/src/reactjs_jsx_ppx.ml +++ b/ppx/src/reactjs_jsx_ppx.ml @@ -457,7 +457,8 @@ let makeExternalDecl fnName loc namedArgListWithKeyAndRef namedTypeList = (* TODO: some line number might still be wrong *) let jsxMapper = - let unit = Exp.construct { txt = Lident "()"; loc = Location.none } None in + let unit = Exp.construct { txt = Lident "()"; loc = Location.none } None + and key_var_txt = "reason_react_ppx_key_arg___x" in let transformUppercaseCall3 ~caller modulePath ~ctxt mapper loc attrs _ callArguments = let children, argsWithLabels = extractChildren callArguments in @@ -499,18 +500,30 @@ let jsxMapper = (Invalid_argument "JSX name can't be the result of function applications") in - let props = - Exp.apply ~loc (Exp.ident ~loc { loc; txt = propsIdent }) propsArg + let component = (nolabel, Exp.ident ~loc { txt = ident; loc }) + and props = + ( nolabel, + Exp.apply ~loc (Exp.ident ~loc { loc; txt = propsIdent }) propsArg ) in - let key_args = - match key with - | Some (label, key) -> - [ (label, mapper#expression ctxt key); (nolabel, unit) ] - | None -> [] - in - Exp.apply ~loc ~attrs jsxExpr - ([ (nolabel, Exp.ident ~loc { txt = ident; loc }); (nolabel, props) ] - @ key_args) + match key with + | Some (label, key) -> + Exp.let_ ~loc Nonrecursive + [ + { + pvb_pat = Pat.var { txt = key_var_txt; loc }; + pvb_expr = mapper#expression ctxt key; + pvb_attributes = []; + pvb_loc = loc; + }; + ] + (Exp.apply ~loc ~attrs jsxExpr + [ + (label, Exp.ident { txt = Lident key_var_txt; loc }); + component; + props; + (nolabel, unit); + ]) + | None -> Exp.apply ~loc ~attrs jsxExpr [ component; props ] in let transformLowercaseCall3 ~ctxt parentExpLoc mapper loc attrs callArguments @@ -522,33 +535,42 @@ let jsxMapper = (fun (arg_label, _) -> "key" = getLabel arg_label) nonChildrenProps in - let jsxExpr, args = - let jsxExpr, key, childrenProp = - jsxExprAndChildren ~ident:"ReactDOM" ~loc:parentExpLoc ~ctxt mapper - ~keyProps children - in - - let propsCall = - Exp.apply ~loc:parentExpLoc - (Exp.ident ~loc:parentExpLoc ~attrs:merlinHideAttrs - { loc; txt = Ldot (Lident "ReactDOM", "domProps") }) - ((match childrenProp with - | Some childrenProp -> - (labelled "children", childrenProp) :: nonChildrenProps - | None -> nonChildrenProps) - |> List.map (fun (label, expression) -> - (label, mapper#expression ctxt expression))) - in - let key_args = - match key with - | Some (label, key) -> - [ (label, mapper#expression ctxt key); (nolabel, unit) ] - | None -> [] - in - ( jsxExpr, - [ (nolabel, componentNameExpr); (nolabel, propsCall) ] @ key_args ) + let jsxExpr, key, childrenProp = + jsxExprAndChildren ~ident:"ReactDOM" ~loc:parentExpLoc ~ctxt mapper + ~keyProps children + in + let propsCall = + Exp.apply ~loc:parentExpLoc + (Exp.ident ~loc:parentExpLoc ~attrs:merlinHideAttrs + { loc; txt = Ldot (Lident "ReactDOM", "domProps") }) + ((match childrenProp with + | Some childrenProp -> + (labelled "children", childrenProp) :: nonChildrenProps + | None -> nonChildrenProps) + |> List.map (fun (label, expression) -> + (label, mapper#expression ctxt expression))) in - Exp.apply ~loc:parentExpLoc ~attrs jsxExpr args + let component = (nolabel, componentNameExpr) + and props = (nolabel, propsCall) in + match key with + | Some (label, key) -> + Exp.let_ ~loc Nonrecursive + [ + { + pvb_pat = Pat.var { txt = key_var_txt; loc }; + pvb_expr = mapper#expression ctxt key; + pvb_attributes = []; + pvb_loc = loc; + }; + ] + (Exp.apply ~loc:parentExpLoc ~attrs jsxExpr + [ + (label, Exp.ident { txt = Lident key_var_txt; loc }); + component; + props; + (nolabel, unit); + ]) + | None -> Exp.apply ~loc:parentExpLoc ~attrs jsxExpr [ component; props ] in let rec recursivelyTransformNamedArgsForMake ~ctxt mapper expr list = From 095786eaea676636e65616a6e0802abde9e14652 Mon Sep 17 00:00:00 2001 From: Antonio Nuno Monteiro Date: Wed, 19 Jul 2023 14:29:54 -0700 Subject: [PATCH 05/13] fix: tests --- ppx/test/output.expected | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ppx/test/output.expected b/ppx/test/output.expected index ff730bf2e..d459438a3 100644 --- a/ppx/test/output.expected +++ b/ppx/test/output.expected @@ -56,7 +56,10 @@ let lower_children_nested = (examples |> (List.map (fun e -> + let reason_react_ppx_key_arg___x + = e.path in ReactDOM.jsxKeyed + ~key:reason_react_ppx_key_arg___x "li" (((ReactDOM.domProps) [@merlin.hide @@ -80,8 +83,6 @@ let lower_children_nested = ReactRouter.push e.path) ())) ()) - ~key:( - e.path) ()))) |> React.list) ())) From 34b9dca0e26dfa607ecad0567b4df95e2965caeb Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Wed, 6 Sep 2023 05:30:32 -0700 Subject: [PATCH 06/13] Add a comment for let --- ppx/src/reactjs_jsx_ppx.ml | 54 ++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/ppx/src/reactjs_jsx_ppx.ml b/ppx/src/reactjs_jsx_ppx.ml index 94027fd02..b4b1b139c 100644 --- a/ppx/src/reactjs_jsx_ppx.ml +++ b/ppx/src/reactjs_jsx_ppx.ml @@ -24,7 +24,7 @@ let constantString ~loc str = let safeTypeFromValue valueStr = let valueStr = getLabel valueStr in match String.sub valueStr 0 1 with "_" -> "T" ^ valueStr | _ -> valueStr - [@@raises Invalid_argument] +[@@raises Invalid_argument] let keyType loc = Typ.constr ~loc { loc; txt = optionIdent } @@ -84,11 +84,11 @@ let extractChildren ?(removeLastPositionUnit = false) propsAndChildren = (Invalid_argument "JSX: found non-labelled argument before the last position") | arg :: rest -> allButLast_ rest (arg :: acc) - [@@raises Invalid_argument] + [@@raises Invalid_argument] in let allButLast lst = allButLast_ lst [] |> List.rev - [@@raises Invalid_argument] + [@@raises Invalid_argument] in match List.partition @@ -111,7 +111,7 @@ let extractChildren ?(removeLastPositionUnit = false) propsAndChildren = | _ -> raise (Invalid_argument "JSX: somehow there's more than one `children` label") - [@@raises Invalid_argument] +[@@raises Invalid_argument] let unerasableIgnore loc = { @@ -158,7 +158,7 @@ let getFnName binding = | { pvb_pat = { ppat_desc = Ppat_var { txt } } } -> txt | _ -> raise (Invalid_argument "react.component calls cannot be destructured.") - [@@raises Invalid_argument] +[@@raises Invalid_argument] let makeNewBinding binding expression newName = match binding with @@ -172,7 +172,7 @@ let makeNewBinding binding expression newName = } | _ -> raise (Invalid_argument "react.component calls cannot be destructured.") - [@@raises Invalid_argument] +[@@raises Invalid_argument] (* Lookup the value of `props` otherwise raise Invalid_argument error *) let getPropsNameValue _acc (loc, exp) = @@ -184,7 +184,7 @@ let getPropsNameValue _acc (loc, exp) = (Invalid_argument ("react.component only accepts props as an option, given: " ^ Longident.last_exn txt)) - [@@raises Invalid_argument] +[@@raises Invalid_argument] (* Lookup the `props` record or string as part of [@react.component] and store the name for use when rewriting *) @@ -212,7 +212,7 @@ let getPropsAttr payload = (Invalid_argument "react.component accepts a record config with props as an options.") | _ -> defaultProps - [@@raises Invalid_argument] +[@@raises Invalid_argument] (* Plucks the label, loc, and type_ from an AST node *) let pluckLabelDefaultLocType (label, default, _, _, loc, type_) = @@ -309,7 +309,7 @@ let rec recursivelyMakeNamedArgsForExternal list args = | _label, Some type_, _ -> type_) args) | [] -> args - [@@raises Invalid_argument] +[@@raises Invalid_argument] (* Build an AST node for the [@bs.obj] representing props for a component *) let makePropsValue fnName loc namedArgListWithKeyAndRef propsType = @@ -337,7 +337,7 @@ let makePropsValue fnName loc namedArgListWithKeyAndRef propsType = ]; pval_loc = loc; } - [@@raises Invalid_argument] +[@@raises Invalid_argument] (* Build an AST node representing an `external` with the definition of the [@bs.obj] *) @@ -348,7 +348,7 @@ let makePropsExternal fnName loc namedArgListWithKeyAndRef propsType = Pstr_primitive (makePropsValue fnName loc namedArgListWithKeyAndRef propsType); } - [@@raises Invalid_argument] +[@@raises Invalid_argument] (* Build an AST node for the signature of the `external` definition *) let makePropsExternalSig fnName loc namedArgListWithKeyAndRef propsType = @@ -357,7 +357,7 @@ let makePropsExternalSig fnName loc namedArgListWithKeyAndRef propsType = psig_desc = Psig_value (makePropsValue fnName loc namedArgListWithKeyAndRef propsType); } - [@@raises Invalid_argument] +[@@raises Invalid_argument] (* Build an AST node for the props name when converted to an object inside the function signature *) @@ -453,7 +453,7 @@ let makeExternalDecl fnName loc namedArgListWithKeyAndRef namedTypeList = makePropsExternal fnName loc (List.map pluckLabelDefaultLocType namedArgListWithKeyAndRef) (makePropsType ~loc namedTypeList) - [@@raises Invalid_argument] +[@@raises Invalid_argument] (* TODO: some line number might still be wrong *) let jsxMapper = @@ -482,7 +482,7 @@ let jsxMapper = let first = String.sub str 0 1 [@@raises Invalid_argument] in let capped = String.uppercase_ascii first in first = capped - [@@raises Invalid_argument] + [@@raises Invalid_argument] in let ident = match modulePath with @@ -507,6 +507,8 @@ let jsxMapper = in match key with | Some (label, key) -> + (* We create a let binding with the value of the key to ensure + the inferred type https://github.com/reasonml/reason-react/pull/752 *) Exp.let_ ~loc Nonrecursive [ { @@ -554,6 +556,8 @@ let jsxMapper = and props = (nolabel, propsCall) in match key with | Some (label, key) -> + (* We create a let binding with the value of the key to ensure + the inferred type https://github.com/reasonml/reason-react/pull/752 *) Exp.let_ ~loc Nonrecursive [ { @@ -647,7 +651,7 @@ let jsxMapper = "react-jsx-ppx: react.component refs only support plain arguments \ and type annotations." | _ -> (list, None) - [@@raises Invalid_argument] + [@@raises Invalid_argument] in let argToType types (name, default, _noLabelName, _alias, loc, type_) = @@ -707,7 +711,7 @@ let jsxMapper = } ) :: types | _ -> types - [@@raises Invalid_argument] + [@@raises Invalid_argument] in let argToConcreteType types (name, loc, type_) = @@ -728,7 +732,7 @@ let jsxMapper = pstr_desc = Pstr_primitive ({ pval_name = { txt = fnName }; pval_attributes; pval_type } as - value_description); + value_description); } as pstr -> ( match List.filter hasAttr pval_attributes with | [] -> structure :: returnStructures @@ -828,7 +832,7 @@ let jsxMapper = "react.component calls can only be on function \ definitions or component wrappers (forwardRef, \ memo).") - [@@raises Invalid_argument] + [@@raises Invalid_argument] in spelunkForFunExpression expression in @@ -1125,7 +1129,7 @@ let jsxMapper = in (Some externalDecl, bindings, newBinding) else (None, [ binding ], None) - [@@raises Invalid_argument] + [@@raises Invalid_argument] in let structuresAndBinding = List.map mapBinding valueBindings in let otherStructures (extern, binding, newBinding) @@ -1158,12 +1162,12 @@ let jsxMapper = ]) @ returnStructures | structure -> structure :: returnStructures - [@@raises Invalid_argument] + [@@raises Invalid_argument] in let reactComponentTransform ~ctxt mapper structures = List.fold_right (transformComponentDefinition ~ctxt mapper) structures [] - [@@raises Invalid_argument] + [@@raises Invalid_argument] in let transformComponentSignature _mapper signature returnSignatures = @@ -1173,7 +1177,7 @@ let jsxMapper = psig_desc = Psig_value ({ pval_name = { txt = fnName }; pval_attributes; pval_type } as - psig_desc); + psig_desc); } as psig -> ( match List.filter hasAttr pval_attributes with | [] -> signature :: returnSignatures @@ -1230,12 +1234,12 @@ let jsxMapper = "Only one react.component call can exist on a component at \ one time")) | signature -> signature :: returnSignatures - [@@raises Invalid_argument] + [@@raises Invalid_argument] in let reactComponentSignatureTransform mapper signatures = List.fold_right (transformComponentSignature mapper) signatures [] - [@@raises Invalid_argument] + [@@raises Invalid_argument] in let transformJsxCall ~ctxt parentExpLoc mapper callExpression callArguments @@ -1275,7 +1279,7 @@ let jsxMapper = (Invalid_argument "JSX: `createElement` should be preceeded by a simple, direct \ module name.") - [@@raises Invalid_argument] + [@@raises Invalid_argument] in object (self) From 117771b8241ec84acdbbeb98e10cbb921e10636e Mon Sep 17 00:00:00 2001 From: Javier Chavarri Date: Wed, 6 Sep 2023 15:34:12 +0000 Subject: [PATCH 07/13] promote tests --- ppx/test/keys.t/run.t | 78 ++++-------------------------------------- ppx/test/lower.t/run.t | 5 +-- 2 files changed, 9 insertions(+), 74 deletions(-) diff --git a/ppx/test/keys.t/run.t b/ppx/test/keys.t/run.t index 871ba58d7..b7798c32c 100644 --- a/ppx/test/keys.t/run.t +++ b/ppx/test/keys.t/run.t @@ -30,18 +30,7 @@ _______^ $ ocamlmerlin single type-enclosing -position 10:14 -verbosity 0 \ > -filename component.re < component.re | jq '.value[0]' - { - "start": { - "line": 10, - "col": 11 - }, - "end": { - "line": 10, - "col": 17 - }, - "type": "Author.t", - "tail": "no" - } + null key={author.Author.name} ______________^ @@ -66,72 +55,28 @@ ____________________^ $ ocamlmerlin single type-enclosing -position 10:28 -verbosity 0 \ > -filename component.re < component.re | jq '.value[0]' - { - "start": { - "line": 10, - "col": 18 - }, - "end": { - "line": 10, - "col": 29 - }, - "type": "string", - "tail": "no" - } + null __^ $ ocamlmerlin single type-enclosing -position 10:41 -verbosity 0 \ > -filename component.re < component.re | jq '.value[0]' - { - "start": { - "line": 10, - "col": 37 - }, - "end": { - "line": 10, - "col": 41 - }, - "type": "string", - "tail": "no" - } + null ______^ $ ocamlmerlin single type-enclosing -position 10:44 -verbosity 0 \ > -filename component.re < component.re | jq '.value[0]' - { - "start": { - "line": 10, - "col": 37 - }, - "end": { - "line": 10, - "col": 73 - }, - "type": "unit", - "tail": "no" - } + null _____________^ $ ocamlmerlin single type-enclosing -position 10:51 -verbosity 0 \ > -filename component.re < component.re | jq '.value[0]' - { - "start": { - "line": 10, - "col": 47 - }, - "end": { - "line": 10, - "col": 53 - }, - "type": "Author.t", - "tail": "no" - } + null ____________________^ @@ -156,15 +101,4 @@ ___________________________^ $ ocamlmerlin single type-enclosing -position 10:66 -verbosity 0 \ > -filename component.re < component.re | jq '.value[0]' - { - "start": { - "line": 10, - "col": 54 - }, - "end": { - "line": 10, - "col": 69 - }, - "type": "string", - "tail": "no" - } + null diff --git a/ppx/test/lower.t/run.t b/ppx/test/lower.t/run.t index 222bd48ee..fce9c3493 100644 --- a/ppx/test/lower.t/run.t +++ b/ppx/test/lower.t/run.t @@ -95,7 +95,9 @@ ~children= examples |> List.map(e => + let reason_react_ppx_key_arg___x = e.path; ReactDOM.jsxKeyed( + ~key=reason_react_ppx_key_arg___x, "li", ([@merlin.hide] ReactDOM.domProps)( ~children= @@ -116,9 +118,8 @@ ), (), ), - ~key=e.path, (), - ) + ); ) |> React.list, (), From 13e940aa4e05143d7f1afea2e28808056da339c9 Mon Sep 17 00:00:00 2001 From: Javier Chavarri Date: Wed, 6 Sep 2023 16:06:05 +0000 Subject: [PATCH 08/13] fix locations --- ppx/reason_react_ppx.ml | 14 ++++---- ppx/test/keys.t/run.t | 78 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 79 insertions(+), 13 deletions(-) diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml index bcec71935..736aa4eb1 100644 --- a/ppx/reason_react_ppx.ml +++ b/ppx/reason_react_ppx.ml @@ -528,10 +528,10 @@ let jsxMapper = | None -> Exp.apply ~loc ~attrs jsxExpr [ component; props ] in - let transformLowercaseCall3 ~ctxt parentExpLoc mapper loc attrs callArguments + let transformLowercaseCall3 ~ctxt parentExpLoc mapper callerLoc attrs callArguments id = let children, nonChildrenProps = extractChildren callArguments in - let componentNameExpr = constantString ~loc id in + let componentNameExpr = constantString ~loc:callerLoc id in let keyProps, nonChildrenProps = List.partition (fun (arg_label, _) -> "key" = getLabel arg_label) @@ -544,7 +544,7 @@ let jsxMapper = let propsCall = Exp.apply ~loc:parentExpLoc (Exp.ident ~loc:parentExpLoc ~attrs:merlinHideAttrs - { loc; txt = Ldot (Lident "ReactDOM", "domProps") }) + { loc = callerLoc; txt = Ldot (Lident "ReactDOM", "domProps") }) ((match childrenProp with | Some childrenProp -> (labelled "children", childrenProp) :: nonChildrenProps @@ -558,18 +558,18 @@ let jsxMapper = | Some (label, key) -> (* We create a let binding with the value of the key to ensure the inferred type https://github.com/reasonml/reason-react/pull/752 *) - Exp.let_ ~loc Nonrecursive + Exp.let_ ~loc:parentExpLoc Nonrecursive [ { - pvb_pat = Pat.var { txt = key_var_txt; loc }; + pvb_pat = Pat.var { txt = key_var_txt; loc = parentExpLoc }; pvb_expr = mapper#expression ctxt key; pvb_attributes = []; - pvb_loc = loc; + pvb_loc = parentExpLoc; }; ] (Exp.apply ~loc:parentExpLoc ~attrs jsxExpr [ - (label, Exp.ident { txt = Lident key_var_txt; loc }); + (label, Exp.ident { txt = Lident key_var_txt; loc = parentExpLoc }); component; props; (nolabel, unit); diff --git a/ppx/test/keys.t/run.t b/ppx/test/keys.t/run.t index b7798c32c..871ba58d7 100644 --- a/ppx/test/keys.t/run.t +++ b/ppx/test/keys.t/run.t @@ -30,7 +30,18 @@ _______^ $ ocamlmerlin single type-enclosing -position 10:14 -verbosity 0 \ > -filename component.re < component.re | jq '.value[0]' - null + { + "start": { + "line": 10, + "col": 11 + }, + "end": { + "line": 10, + "col": 17 + }, + "type": "Author.t", + "tail": "no" + } key={author.Author.name} ______________^ @@ -55,28 +66,72 @@ ____________________^ $ ocamlmerlin single type-enclosing -position 10:28 -verbosity 0 \ > -filename component.re < component.re | jq '.value[0]' - null + { + "start": { + "line": 10, + "col": 18 + }, + "end": { + "line": 10, + "col": 29 + }, + "type": "string", + "tail": "no" + } __^ $ ocamlmerlin single type-enclosing -position 10:41 -verbosity 0 \ > -filename component.re < component.re | jq '.value[0]' - null + { + "start": { + "line": 10, + "col": 37 + }, + "end": { + "line": 10, + "col": 41 + }, + "type": "string", + "tail": "no" + } ______^ $ ocamlmerlin single type-enclosing -position 10:44 -verbosity 0 \ > -filename component.re < component.re | jq '.value[0]' - null + { + "start": { + "line": 10, + "col": 37 + }, + "end": { + "line": 10, + "col": 73 + }, + "type": "unit", + "tail": "no" + } _____________^ $ ocamlmerlin single type-enclosing -position 10:51 -verbosity 0 \ > -filename component.re < component.re | jq '.value[0]' - null + { + "start": { + "line": 10, + "col": 47 + }, + "end": { + "line": 10, + "col": 53 + }, + "type": "Author.t", + "tail": "no" + } ____________________^ @@ -101,4 +156,15 @@ ___________________________^ $ ocamlmerlin single type-enclosing -position 10:66 -verbosity 0 \ > -filename component.re < component.re | jq '.value[0]' - null + { + "start": { + "line": 10, + "col": 54 + }, + "end": { + "line": 10, + "col": 69 + }, + "type": "string", + "tail": "no" + } From 5815a290eaadb9b0e7963315d013d85f4dcf2701 Mon Sep 17 00:00:00 2001 From: Javier Chavarri Date: Wed, 6 Sep 2023 16:07:04 +0000 Subject: [PATCH 09/13] rename key txt to `Key` --- ppx/reason_react_ppx.ml | 2 +- ppx/test/lower.t/run.t | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml index 736aa4eb1..b2552d061 100644 --- a/ppx/reason_react_ppx.ml +++ b/ppx/reason_react_ppx.ml @@ -458,7 +458,7 @@ let makeExternalDecl fnName loc namedArgListWithKeyAndRef namedTypeList = (* TODO: some line number might still be wrong *) let jsxMapper = let unit = Exp.construct { txt = Lident "()"; loc = Location.none } None - and key_var_txt = "reason_react_ppx_key_arg___x" in + and key_var_txt = "Key" in let transformUppercaseCall3 ~caller modulePath ~ctxt mapper loc attrs _ callArguments = let children, argsWithLabels = extractChildren callArguments in diff --git a/ppx/test/lower.t/run.t b/ppx/test/lower.t/run.t index fce9c3493..0898fb7b4 100644 --- a/ppx/test/lower.t/run.t +++ b/ppx/test/lower.t/run.t @@ -95,9 +95,9 @@ ~children= examples |> List.map(e => - let reason_react_ppx_key_arg___x = e.path; + let Key = e.path; ReactDOM.jsxKeyed( - ~key=reason_react_ppx_key_arg___x, + ~key=Key, "li", ([@merlin.hide] ReactDOM.domProps)( ~children= From f8582d8771a1e96c34824dcec0eb6afb6dd75f78 Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Thu, 7 Sep 2023 08:48:39 -0700 Subject: [PATCH 10/13] Remove old expected snapshot --- ppx/test/output.expected | 420 --------------------------------------- 1 file changed, 420 deletions(-) delete mode 100644 ppx/test/output.expected diff --git a/ppx/test/output.expected b/ppx/test/output.expected deleted file mode 100644 index c63dc5899..000000000 --- a/ppx/test/output.expected +++ /dev/null @@ -1,420 +0,0 @@ -let lower = ReactDOM.jsx "div" (((ReactDOM.domProps)[@merlin.hide ]) ()) -let lower_empty_attr = - ReactDOM.jsx "div" (((ReactDOM.domProps)[@merlin.hide ]) ~className:"" ()) -let lower_inline_styles = - ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) - ~style:(ReactDOM.Style.make ~backgroundColor:"gainsboro" ()) ()) -let lower_inner_html = - ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) - ~dangerouslySetInnerHTML:([%bs.obj { __html = text }]) ()) -let lower_opt_attr = - ReactDOM.jsx "div" (((ReactDOM.domProps)[@merlin.hide ]) ?tabIndex ()) -let lowerWithChildAndProps foo = - ReactDOM.jsx "a" - (((ReactDOM.domProps)[@merlin.hide ]) ~children:foo ~tabIndex:1 - ~href:"https://example.com" ()) -let lower_child_static = - ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:(ReactDOM.jsx "span" - (((ReactDOM.domProps)[@merlin.hide ]) ())) ()) -let lower_child_ident = - ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) ~children:lolaspa ()) -let lower_child_single = - ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:(ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) ())) ()) -let lower_children_multiple foo bar = - ReactDOM.jsxs "lower" - (((ReactDOM.domProps)[@merlin.hide ]) ~children:(React.array [|foo;bar|]) - ()) -let lower_child_with_upper_as_children = - ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:(React.jsx App.make (App.makeProps ())) ()) -let lower_children_nested = - ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:(ReactDOM.jsxs "div" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:(React.array - [|(ReactDOM.jsx "h2" - (((ReactDOM.domProps) - [@merlin.hide ]) - ~children:("jsoo-react" |> s) - ~className:"title" ()));( - ReactDOM.jsx "nav" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:(ReactDOM.jsx "ul" - (((ReactDOM.domProps) - [@merlin.hide ]) - ~children:( - (examples |> - (List.map - (fun e -> - let reason_react_ppx_key_arg___x - = e.path in - ReactDOM.jsxKeyed - ~key:reason_react_ppx_key_arg___x - "li" - (((ReactDOM.domProps) - [@merlin.hide - ]) - ~children:( - ReactDOM.jsx - "a" - (((ReactDOM.domProps) - [@merlin.hide - ]) - ~children:( - e.title - |> s) - ~href:( - e.path) - ~onClick:( - fun event - -> - ReactEvent.Mouse.preventDefault - event; - ReactRouter.push - e.path) - ())) ()) - ()))) - |> React.list) - ())) - ~className:"menu" ()))|]) - ~className:"sidebar" ())) ~className:"flex-container" - ()) -let fragment foo = ((ReactDOM.createElement React.jsxFragment [|foo|]) - [@bla ]) -let poly_children_fragment foo bar = - ReactDOM.createElement React.jsxFragment [|foo;bar|] -let nested_fragment foo bar baz = - ReactDOM.createElement React.jsxFragment - [|foo;(ReactDOM.createElement React.jsxFragment [|bar;baz|])|] -let nested_fragment_with_lower foo = - ReactDOM.createElement React.jsxFragment - [|(ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) ~children:foo ()))|] -let upper = React.jsx Upper.make (Upper.makeProps ()) -let upper_prop = React.jsx Upper.make (Upper.makeProps ~count ()) -let upper_children_single foo = - React.jsx Upper.make (Upper.makeProps ~children:foo ()) -let upper_children_multiple foo bar = - React.jsxs Upper.make - (Upper.makeProps ~children:(React.array [|foo;bar|]) ()) -let upper_children = - React.jsx Page.make - (Page.makeProps - ~children:(ReactDOM.jsx "h1" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:(React.string "Yep") ())) ~moreProps:"hgalo" - ()) -let upper_nested_module = - React.jsx Foo.Bar.make (Foo.Bar.makeProps ~a:1 ~b:"1" ()) -let upper_child_expr = - React.jsx Div.make (Div.makeProps ~children:(React.int 1) ()) -let upper_child_ident = React.jsx Div.make (Div.makeProps ~children:lola ()) -let upper_all_kinds_of_props = - React.jsx MyComponent.make - (MyComponent.makeProps - ~children:(ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) ~children:"hello" - ())) ~booleanAttribute:true ~stringAttribute:"string" - ~intAttribute:1 ?forcedOptional:((Some "hello")[@explicit_arity ]) - ~onClick:(send handleClick) ()) -let upper_ref_with_children = - React.jsx FancyButton.make - (FancyButton.makeProps - ~children:(ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) ())) ~ref:buttonRef - ()) -let lower_ref_with_children = - ReactDOM.jsx "button" - (((ReactDOM.domProps)[@merlin.hide ]) ~children ~ref - ~className:"FancyButton" ()) -let lower_with_many_props = - ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:(ReactDOM.jsxs "picture" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:(React.array - [|(ReactDOM.jsx "img" - (((ReactDOM.domProps) - [@merlin.hide ]) - ~src:"picture/img.png" - ~alt:"test picture/img.png" - ~id:"idimg" ()));(ReactDOM.jsx - "source" - (((ReactDOM.domProps) - [@merlin.hide - ]) - ~type_:"image/webp" - ~src:"picture/img1.webp" - ()));( - ReactDOM.jsx "source" - (((ReactDOM.domProps)[@merlin.hide ]) - ~type_:"image/jpeg" - ~src:"picture/img2.jpg" ()))|]) - ~id:"idpicture" ())) ~translate:"yes" ()) -let some_random_html_element = - ReactDOM.jsx "text" - (((ReactDOM.domProps)[@merlin.hide ]) ~dx:"1 2" ~dy:"3 4" ()) -module React_component_with_props = - struct - external makeProps : - lola:'lola -> ?key:string -> unit -> < lola: 'lola > Js.t = "" - [@@bs.obj ] - let make = - ((fun ~lola -> - ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:(React.string lola) ())) - [@warning "-16"]) - let make = - let Generated$React_component_with_props - (Props : < lola: 'lola > Js.t) = make ~lola:(Props ## lola) in - Generated$React_component_with_props - end -let react_component_with_props = - React.jsx React_component_with_props.make - (React_component_with_props.makeProps ~lola:"flores" ()) -module Upper_case_with_fragment_as_root = - struct - external makeProps : - ?name:'name -> ?key:string -> unit -> < name: 'name option > Js.t = - ""[@@bs.obj ] - let make = - ((fun ?(name= "") -> - ReactDOM.createElement React.jsxFragment - [|(ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:(React.string ("First " ^ name)) ()));( - React.jsx Hello.make - (Hello.makeProps ~children:(React.string ("2nd " ^ name)) - ~one:"1" ()))|]) - [@warning "-16"]) - let make = - let Generated$Upper_case_with_fragment_as_root - (Props : < name: 'name option > Js.t) = make ?name:(Props ## name) in - Generated$Upper_case_with_fragment_as_root - end -module Using_React_memo = - struct - external makeProps : - a:'a -> ?key:string -> unit -> < a: 'a > Js.t = ""[@@bs.obj ] - let make = - ((fun ~a -> - ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:((Printf.sprintf "`a` is %s" a) |> React.string) ())) - [@warning "-16"]) - let make = - React.memo - (let Generated$Using_React_memo (Props : < a: 'a > Js.t) = - make ~a:(Props ## a) in - Generated$Using_React_memo) - end -module Using_memo_custom_compare_Props = - struct - external makeProps : - a:'a -> ?key:string -> unit -> < a: 'a > Js.t = ""[@@bs.obj ] - let make = - React.memoCustomCompareProps - (fun ~a -> - ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:((Printf.sprintf "`a` is %d" a) |> React.string) ())) - (fun prevPros -> fun nextProps -> false) - let make = - let Generated$Using_memo_custom_compare_Props - (Props : < a: 'a > Js.t) = make ~a:(Props ## a) in - Generated$Using_memo_custom_compare_Props - end -module Forward_Ref = - struct - external makeProps : - children:'children -> - buttonRef:'buttonRef -> - ?key:string -> - unit -> < children: 'children ;buttonRef: 'buttonRef > Js.t = - ""[@@bs.obj ] - let make = - ((fun ~children -> - ((fun ~buttonRef -> - ReactDOM.jsx "button" - (((ReactDOM.domProps)[@merlin.hide ]) ~children - ~ref:buttonRef ~className:"FancyButton" ())) - [@warning "-16"])) - [@warning "-16"]) - let make = - React.forwardRef - (let Generated$Forward_Ref - (Props : < children: 'children ;buttonRef: 'buttonRef > Js.t) - = - make ~buttonRef:(Props ## buttonRef) ~children:(Props ## children) in - Generated$Forward_Ref) - end -module Onclick_handler_button = - struct - external makeProps : - name:'name -> - ?isDisabled:'isDisabled -> - ?key:string -> - unit -> < name: 'name ;isDisabled: 'isDisabled option > Js.t - = ""[@@bs.obj ] - let make = - ((fun ~name -> - ((fun ?isDisabled -> - let onClick event = Js.log event in - ReactDOM.jsx "button" - (((ReactDOM.domProps)[@merlin.hide ]) ~name ~onClick - ~disabled:isDisabled ())) - [@warning "-16"])) - [@warning "-16"]) - let make = - let Generated$Onclick_handler_button - (Props : < name: 'name ;isDisabled: 'isDisabled option > Js.t) = - make ?isDisabled:(Props ## isDisabled) ~name:(Props ## name) in - Generated$Onclick_handler_button - end -module Children_as_string = - struct - external makeProps : - ?name:'name -> ?key:string -> unit -> < name: 'name option > Js.t = - ""[@@bs.obj ] - let make = - ((fun ?(name= "joe") -> - ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:((Printf.sprintf "`name` is %s" name) |> - React.string) ())) - [@warning "-16"]) - let make = - let Generated$Children_as_string - (Props : < name: 'name option > Js.t) = make ?name:(Props ## name) in - Generated$Children_as_string - end -let () = Dream.run () -let l = 33 -module Uppercase_with_SSR_components = - struct - external makeProps : - children:'children -> - moreProps:'moreProps -> - ?key:string -> - unit -> < children: 'children ;moreProps: 'moreProps > Js.t = - ""[@@bs.obj ] - let make = - ((fun ~children -> - ((fun ~moreProps -> - ReactDOM.jsxs "html" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:(React.array - [|(ReactDOM.jsx "head" - (((ReactDOM.domProps)[@merlin.hide ]) - ~children:(ReactDOM.jsx "title" - (((ReactDOM.domProps) - [@merlin.hide ]) - ~children:(React.string - ("SSR React " - ^ - moreProps)) - ())) ()));(ReactDOM.jsxs - "body" - (((ReactDOM.domProps) - [@merlin.hide - ]) - ~children:( - React.array - [|( - ReactDOM.jsx - "div" - (((ReactDOM.domProps) - [@merlin.hide - ]) - ~children - ~id:"root" - ()));( - ReactDOM.jsx - "script" - (((ReactDOM.domProps) - [@merlin.hide - ]) - ~src:"/static/client.js" - ()))|]) - ()))|]) - ())) - [@warning "-16"])) - [@warning "-16"]) - let make = - let Generated$Uppercase_with_SSR_components - (Props : < children: 'children ;moreProps: 'moreProps > Js.t) = - make ~moreProps:(Props ## moreProps) ~children:(Props ## children) in - Generated$Uppercase_with_SSR_components - end -module Upper_with_aria = - struct - external makeProps : - children:'children -> - ?key:string -> unit -> < children: 'children > Js.t = ""[@@bs.obj - ] - let make = - ((fun ~children -> - ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) ~children - ~ariaHidden:"true" ())) - [@warning "-16"]) - let make = - let Generated$Upper_with_aria (Props : < children: 'children > Js.t) - = make ~children:(Props ## children) in - Generated$Upper_with_aria - end -let data_attributes_should_transform_to_kebabcase = - ReactDOM.createElement React.jsxFragment - [|(ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) ~dataAttribute:"" - ~dataattribute:"" ~className:"md:w-1/3" ()));(ReactDOM.jsx "div" - (((ReactDOM.domProps) - [@merlin.hide - ]) - ~className:"md:w-2/3" - ()))|] -let render_onclickPropsAsString = - ReactDOM.jsx "div" - (((ReactDOM.domProps)[@merlin.hide ]) ~_onclick:"alert('hello')" ()) -module External = - struct - external componentProps : - a:int -> - b:string -> ?key:string -> unit -> < a: int ;b: string > Js.t = - ""[@@bs.obj ] - external component : - (< a: int ;b: string > Js.t, React.element) React.componentLike = - "require(\"my-react-library\").MyReactComponent"[@@otherAttribute - "bla"] - end -module type X_int = sig val x : int end -module Func(M:X_int) = - struct - let x = M.x + 1 - external makeProps : - a:'a -> b:'b -> ?key:string -> unit -> < a: 'a ;b: 'b > Js.t = "" - [@@bs.obj ] - let make = - ((fun ~a -> - ((fun ~b -> - fun _ -> - print_endline "This function should be named `Test$Func`" M.x; - ReactDOM.jsx "div" (((ReactDOM.domProps)[@merlin.hide ]) ())) - [@warning "-16"])) - [@warning "-16"]) - let make = - let Generated$Func (Props : < a: 'a ;b: 'b > Js.t) = - make ~b:(Props ## b) ~a:(Props ## a) () in - Generated$Func - end From bf0a4780c73d7938967ff7cc126a836e8a14847b Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Thu, 7 Sep 2023 11:45:30 -0700 Subject: [PATCH 11/13] Fix location on key --- ppx/reason_react_ppx.ml | 113 ++++++++++++++++++++++++---------------- test/React__test.re | 4 +- 2 files changed, 69 insertions(+), 48 deletions(-) diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml index a8587f502..53ed4048f 100644 --- a/ppx/reason_react_ppx.ml +++ b/ppx/reason_react_ppx.ml @@ -71,9 +71,9 @@ module Binding = struct { loc; txt = Ldot (Lident "ReactDOM", "createElement") }) [ (nolabel, element); (nolabel, children) ] - let domProps ~parentExpLoc ~loc props = - Builder.pexp_apply ~loc:parentExpLoc - (Builder.pexp_ident ~loc:parentExpLoc ~attrs:merlinHideAttrs + let domProps ~applyLoc ~loc props = + Builder.pexp_apply ~loc:applyLoc + (Builder.pexp_ident ~loc:applyLoc ~attrs:merlinHideAttrs { loc; txt = Ldot (Lident "ReactDOM", "domProps") }) props end @@ -508,7 +508,7 @@ let jsxMapper = { txt = Lident "()"; loc = Location.none } None and key_var_txt = "Key" in - let transformUppercaseCall3 ~caller modulePath ~ctxt mapper loc attrs _ + let transformUppercaseCall3 ~caller ~ctxt modulePath mapper parentExpLoc attrs callArguments = let children, argsWithLabels = extractChildren callArguments in let argsForMake = argsWithLabels in @@ -518,7 +518,7 @@ let jsxMapper = argsForMake in let jsxExpr, key, childrenProp = - reactJsxExprAndChildren ~loc ~ctxt mapper ~keyProps children + reactJsxExprAndChildren ~loc:parentExpLoc ~ctxt ~keyProps mapper children in let propsArg = (match childrenProp with @@ -549,19 +549,24 @@ let jsxMapper = (Invalid_argument "JSX name can't be the result of function applications") in - let component = (nolabel, Builder.pexp_ident ~loc { txt = ident; loc }) + let component = + ( nolabel, + Builder.pexp_ident ~loc:parentExpLoc { txt = ident; loc = parentExpLoc } + ) and props = - (nolabel, - Builder.pexp_apply ~loc - (Builder.pexp_ident ~loc { loc; txt = propsIdent }) - propsArg - ) + ( nolabel, + Builder.pexp_apply ~loc:parentExpLoc + (Builder.pexp_ident ~loc:parentExpLoc + { loc = parentExpLoc; txt = propsIdent }) + propsArg ) in match key with - | None -> Builder.pexp_apply ~loc ~attrs jsxExpr [ component; props ] - | Some (label, key) -> + | None -> + Builder.pexp_apply ~loc:parentExpLoc ~attrs jsxExpr [ component; props ] + | Some (label, key) -> (* We create a let binding with the value of the key to ensure - the inferred type https://github.com/reasonml/reason-react/pull/752 *) + the inferred type https://github.com/reasonml/reason-react/pull/752 *) + let loc = parentExpLoc in Builder.pexp_let ~loc Nonrecursive [ { @@ -571,17 +576,19 @@ let jsxMapper = pvb_loc = loc; }; ] - (Builder.pexp_apply ~loc ~attrs jsxExpr + (Builder.pexp_apply ~loc:parentExpLoc ~attrs jsxExpr [ - (label, Builder.pexp_ident ~loc { txt = Lident key_var_txt; loc }); + ( label, + Builder.pexp_ident ~loc:parentExpLoc + { txt = Lident key_var_txt; loc = parentExpLoc } ); component; props; (nolabel, unit); ]) in - let transformLowercaseCall3 ~ctxt parentExpLoc mapper callerLoc attrs callArguments - id = + let transformLowercaseCall3 ~ctxt parentExpLoc mapper callerLoc attrs + callArguments id = let children, nonChildrenProps = extractChildren callArguments in let componentNameExpr = constantString ~loc:callerLoc id in let keyProps, nonChildrenProps = @@ -589,33 +596,47 @@ let jsxMapper = (fun (arg_label, _) -> "key" = getLabel arg_label) nonChildrenProps in - let jsxExpr, args = - let jsxExpr, key, childrenProp = - reactDomJsxExprAndChildren ~loc:parentExpLoc ~ctxt mapper ~keyProps - children - in - let props = - (match childrenProp with - | Some childrenProp -> - (labelled "children", childrenProp) :: nonChildrenProps - | None -> nonChildrenProps) - |> List.map (fun (label, expression) -> - (label, mapper#expression ctxt expression)) - in - let key_args = - match key with - | Some (label, key) -> - [ (label, mapper#expression ctxt key); (nolabel, unit) ] - | None -> [] - in - ( jsxExpr, - [ - (nolabel, componentNameExpr); - (nolabel, Binding.ReactDOM.domProps ~parentExpLoc ~loc:callerLoc props); - ] - @ key_args ) + + let jsxExpr, key, childrenProp = + reactDomJsxExprAndChildren ~loc:parentExpLoc ~ctxt mapper ~keyProps + children + in + let props = + (match childrenProp with + | Some childrenProp -> + (labelled "children", childrenProp) :: nonChildrenProps + | None -> nonChildrenProps) + |> List.map (fun (label, expression) -> + (label, mapper#expression ctxt expression)) + in + let component = (nolabel, componentNameExpr) + and props = + ( nolabel, + Binding.ReactDOM.domProps ~applyLoc:parentExpLoc ~loc:callerLoc props ) in - Builder.pexp_apply ~loc:parentExpLoc ~attrs jsxExpr args + let loc = parentExpLoc in + let gloc = { loc with loc_ghost = true } in + match key with + | Some (label, key) -> + (* We create a let binding with the value of the key to ensure + the inferred type https://github.com/reasonml/reason-react/pull/752 *) + Builder.pexp_let ~loc:gloc Nonrecursive + [ + { + pvb_pat = Builder.ppat_var ~loc { txt = key_var_txt; loc }; + pvb_expr = mapper#expression ctxt key; + pvb_attributes = []; + pvb_loc = loc; + }; + ] + (Builder.pexp_apply ~loc ~attrs jsxExpr + [ + (label, Builder.pexp_ident ~loc { txt = Lident key_var_txt; loc }); + component; + props; + (nolabel, unit); + ]) + | None -> Builder.pexp_apply ~loc ~attrs jsxExpr [ component; props ] in let rec recursivelyTransformNamedArgsForMake ~ctxt mapper expr list = @@ -1308,7 +1329,7 @@ let jsxMapper = (* Foo.createElement(~prop1=foo, ~prop2=bar, ~children=[], ()) *) | { txt = Ldot (modulePath, ("createElement" | "make")) } -> transformUppercaseCall3 ~ctxt ~caller:"make" modulePath mapper - parentExpLoc attrs callExpression callArguments + parentExpLoc attrs callArguments (* div(~prop1=foo, ~prop2=bar, ~children=[bla], ()) *) (* turn that into ReactDOM.createElement("div", ~props=ReactDOM.domProps(~props1=foo, @@ -1321,7 +1342,7 @@ let jsxMapper = https://github.com/reasonml/reason/pull/2541 *) | { txt = Ldot (modulePath, anythingNotCreateElementOrMake) } -> transformUppercaseCall3 ~ctxt ~caller:anythingNotCreateElementOrMake - modulePath mapper parentExpLoc attrs callExpression callArguments + modulePath mapper parentExpLoc attrs callArguments | { txt = Lapply _ } -> (* don't think there's ever a case where this is reached *) raise diff --git a/test/React__test.re b/test/React__test.re index f6d2d59b5..563048f41 100644 --- a/test/React__test.re +++ b/test/React__test.re @@ -8,7 +8,7 @@ module DummyStatefulComponent = { let make = (~initialValue=0, ()) => { let (value, setValue) = React.useState(() => initialValue); - ; }; @@ -287,7 +287,7 @@ describe("React", () => { test("can render react components", () => { let container = getContainer(container); - act(() => {ReactDOM.render(, container)}); + act(() => {ReactDOM.render(, container)}); expect( container From 151c89bb79e0495dfc3e3dd91de7048cb5260803 Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Thu, 7 Sep 2023 11:45:40 -0700 Subject: [PATCH 12/13] Specify 0.26.0 in ocamlformat --- .ocamlformat | 2 +- ppx/test/keys.t/run.t | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.ocamlformat b/.ocamlformat index 3fe595c05..ed7d4b31d 100644 --- a/.ocamlformat +++ b/.ocamlformat @@ -1 +1 @@ -version = 0.24.0 +version = 0.26.0 diff --git a/ppx/test/keys.t/run.t b/ppx/test/keys.t/run.t index d46dadc87..3242dd6b0 100644 --- a/ppx/test/keys.t/run.t +++ b/ppx/test/keys.t/run.t @@ -32,7 +32,7 @@ _^ "line": 10, "col": 85 }, - "type": "ReactDOM.domProps", + "type": "string", "tail": "no" } From 25f32b7296c5a4cf2cb8df73cc75c8b8bbadd1ea Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Thu, 7 Sep 2023 12:25:12 -0700 Subject: [PATCH 13/13] Specify merlin to 4.10-414 --- dune-project | 4 ++++ reason-react.opam | 1 + 2 files changed, 5 insertions(+) diff --git a/dune-project b/dune-project index f18444141..993da5cdc 100644 --- a/dune-project +++ b/dune-project @@ -41,6 +41,10 @@ (reason (>= 3.6.0)) (ocaml-lsp-server :with-test) + (merlin + (and + (= 4.10-414) + :with-test)) (opam-check-npm-deps (and (= 1.0.0) diff --git a/reason-react.opam b/reason-react.opam index a3b4ba00b..7747e9488 100644 --- a/reason-react.opam +++ b/reason-react.opam @@ -23,6 +23,7 @@ depends: [ "reason-react-ppx" "reason" {>= "3.6.0"} "ocaml-lsp-server" {with-test} + "merlin" {= "4.10-414" & with-test} "opam-check-npm-deps" {= "1.0.0" & with-dev-setup} "ocamlformat" {= "0.24.0" & with-dev-setup} "odoc" {with-doc}