From bc5e4675b3fcbe66d58943da4c4a604226efd026 Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Tue, 16 Jul 2024 16:23:06 +0200 Subject: [PATCH 01/14] Add test for error on key --- ppx/test/key-as-prop-error.t/component.re | 2 ++ ppx/test/key-as-prop-error.t/run.t | 17 +++++++++++++++++ 2 files changed, 19 insertions(+) create mode 100644 ppx/test/key-as-prop-error.t/component.re create mode 100644 ppx/test/key-as-prop-error.t/run.t diff --git a/ppx/test/key-as-prop-error.t/component.re b/ppx/test/key-as-prop-error.t/component.re new file mode 100644 index 000000000..1ce26b19b --- /dev/null +++ b/ppx/test/key-as-prop-error.t/component.re @@ -0,0 +1,2 @@ +[@react.component] +let make = (~key) =>
key->React.string
; diff --git a/ppx/test/key-as-prop-error.t/run.t b/ppx/test/key-as-prop-error.t/run.t new file mode 100644 index 000000000..77ecf31d0 --- /dev/null +++ b/ppx/test/key-as-prop-error.t/run.t @@ -0,0 +1,17 @@ +Test some locations in reason-react components + + $ cat >dune-project < (lang dune 3.8) + > (using melange 0.1) + > EOF + + $ cat >dune < (melange.emit + > (alias foo) + > (target foo) + > (libraries reason-react) + > (preprocess + > (pps melange.ppx reason-react-ppx))) + > EOF + + $ dune build From d12262d7c7b775380ac89e6d3433ac58b073f108 Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Tue, 16 Jul 2024 16:23:26 +0200 Subject: [PATCH 02/14] Add locations into key --- ppx/reason_react_ppx.ml | 6 ++---- ppx/test/key-as-prop-error.t/run.t | 5 +++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml index cbceb3437..d67c699d6 100644 --- a/ppx/reason_react_ppx.ml +++ b/ppx/reason_react_ppx.ml @@ -641,11 +641,9 @@ let jsxMapper = let rec recursivelyTransformNamedArgsForMake ~ctxt mapper expr list = let expr = mapper#expression ctxt expr in match expr.pexp_desc with - (* TODO: make this show up with a loc. *) | Pexp_fun (Labelled "key", _, _, _) | Pexp_fun (Optional "key", _, _, _) -> - raise - (Invalid_argument - "Key cannot be accessed inside of a component. Don't worry - you \ + Location.raise_errorf ~loc:expr.pexp_loc + ("Key cannot be accessed inside of a component. Don't worry - you \ can always key a component from its parent!") | Pexp_fun (Labelled "ref", _, _, _) | Pexp_fun (Optional "ref", _, _, _) -> raise diff --git a/ppx/test/key-as-prop-error.t/run.t b/ppx/test/key-as-prop-error.t/run.t index 77ecf31d0..83c3c0e58 100644 --- a/ppx/test/key-as-prop-error.t/run.t +++ b/ppx/test/key-as-prop-error.t/run.t @@ -15,3 +15,8 @@ Test some locations in reason-react components > EOF $ dune build + File "component.re", line 2, characters 11-51: + 2 | let make = (~key) =>
key->React.string
; + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + Error: Key cannot be accessed inside of a component. Don't worry - you can always key a component from its parent! + [1] From cfdd71222e4131129c2892eefa63c71cf52a92ac Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Tue, 16 Jul 2024 16:25:55 +0200 Subject: [PATCH 03/14] Remove restriction for ref as prop --- ppx/reason_react_ppx.ml | 5 ----- ppx/test/component.t/input.re | 35 +++++++++++++++++++++-------------- ppx/test/component.t/run.t | 21 +++++++++++++++++++++ 3 files changed, 42 insertions(+), 19 deletions(-) diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml index d67c699d6..d563916b7 100644 --- a/ppx/reason_react_ppx.ml +++ b/ppx/reason_react_ppx.ml @@ -645,11 +645,6 @@ let jsxMapper = Location.raise_errorf ~loc:expr.pexp_loc ("Key cannot be accessed inside of a component. Don't worry - you \ can always key a component from its parent!") - | Pexp_fun (Labelled "ref", _, _, _) | Pexp_fun (Optional "ref", _, _, _) -> - raise - (Invalid_argument - "Ref cannot be passed as a normal prop. Please use `forwardRef` \ - API instead.") | Pexp_fun ( ((Optional label | Labelled label) as arg), default, diff --git a/ppx/test/component.t/input.re b/ppx/test/component.t/input.re index 7fdccd901..a34b748c6 100644 --- a/ppx/test/component.t/input.re +++ b/ppx/test/component.t/input.re @@ -17,21 +17,21 @@ module Upper_case_with_fragment_as_root = { }; /* module Using_React_memo = { - [@react.component] - let make = - React.memo((~a) => -
{Printf.sprintf("`a` is %s", a) |> React.string}
- ); -}; + [@react.component] + let make = + React.memo((~a) => +
{Printf.sprintf("`a` is %s", a) |> React.string}
+ ); + }; -module Using_memo_custom_compare_Props = { - [@react.component] - let make = - React.memoCustomCompareProps( - (~a) =>
{Printf.sprintf("`a` is %d", a) |> React.string}
, - (prevPros, nextProps) => false, - ); -}; */ + module Using_memo_custom_compare_Props = { + [@react.component] + let make = + React.memoCustomCompareProps( + (~a) =>
{Printf.sprintf("`a` is %d", a) |> React.string}
, + (prevPros, nextProps) => false, + ); + }; */ module Forward_Ref = { [@react.component] @@ -41,6 +41,13 @@ module Forward_Ref = { }); }; +module Ref_as_prop = { + [@react.component] + let make = (~children, ~ref) => { + ; + }; +}; + module Onclick_handler_button = { [@react.component] let make = (~name, ~isDisabled=?) => { diff --git a/ppx/test/component.t/run.t b/ppx/test/component.t/run.t index 29ff6dd64..e3c4583a3 100644 --- a/ppx/test/component.t/run.t +++ b/ppx/test/component.t/run.t @@ -68,6 +68,27 @@ We need to output ML syntax here, otherwise refmt could not parse it. make ~buttonRef:(Props ## buttonRef) ~children:(Props ## children) in Output$Forward_Ref) end + module Ref_as_prop = + struct + external makeProps : + children:'children -> + ref:'ref -> + ?key:string -> unit -> < children: 'children ;ref: 'ref > Js.t + = ""[@@mel.obj ] + let make = + ((fun ~children -> + ((fun ~ref -> + ReactDOM.jsx "button" + (((ReactDOM.domProps)[@merlin.hide ]) ~children ~ref + ~className:"FancyButton" ())) + [@warning "-16"])) + [@warning "-16"]) + let make = + let Output$Ref_as_prop + (Props : < children: 'children ;ref: 'ref > Js.t) = + make ~ref:(Props ## ref) ~children:(Props ## children) in + Output$Ref_as_prop + end module Onclick_handler_button = struct external makeProps : From 8f846cc5e98c8e3f83031cbf015bf747821e3b7b Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Tue, 16 Jul 2024 16:26:08 +0200 Subject: [PATCH 04/14] Format .re files from cram tests --- ppx/test/keys.t/component.re | 4 +++- ppx/test/uppercase.t/component.re | 8 ++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/ppx/test/keys.t/component.re b/ppx/test/keys.t/component.re index 2e0191c23..632c307ab 100644 --- a/ppx/test/keys.t/component.re +++ b/ppx/test/keys.t/component.re @@ -7,4 +7,6 @@ module Author = { [@react.component] let make = (~author) => - ; + + + ; diff --git a/ppx/test/uppercase.t/component.re b/ppx/test/uppercase.t/component.re index 708a50787..bcb65c468 100644 --- a/ppx/test/uppercase.t/component.re +++ b/ppx/test/uppercase.t/component.re @@ -1,17 +1,13 @@ module Box = { [@react.component] let make = (~children) => { -
- {children} -
; +
children
; }; }; module Uppercase = { [@react.component] let make = (~children) => { - - {children} - ; + children ; }; }; From 6f9b29a8d767f728489f92ce67d5168e802142d9 Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Tue, 16 Jul 2024 16:26:25 +0200 Subject: [PATCH 05/14] Format all .re files --- src/ReactDOMServer.rei | 2 -- test/jest/Expect.re | 3 ++- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/ReactDOMServer.rei b/src/ReactDOMServer.rei index b967a1465..95a1cd02d 100644 --- a/src/ReactDOMServer.rei +++ b/src/ReactDOMServer.rei @@ -4,5 +4,3 @@ external renderToString: React.element => string = "renderToString"; [@mel.module "react-dom/server"] external renderToStaticMarkup: React.element => string = "renderToStaticMarkup"; - - diff --git a/test/jest/Expect.re b/test/jest/Expect.re index 71d9da380..0b1e0a6a0 100644 --- a/test/jest/Expect.re +++ b/test/jest/Expect.re @@ -17,7 +17,8 @@ external toBeGreaterThanOrEqual: (t('a), 'a) => unit = "toBeGreaterThanOrEqual"; [@mel.send] external toBeLessThanOrEqual: (t('a), 'a) => unit = "toBeLessThanOrEqual"; -[@mel.send] external toHaveLength: (t(array('a)), 'a) => unit = "toHaveLength"; +[@mel.send] +external toHaveLength: (t(array('a)), 'a) => unit = "toHaveLength"; [@mel.get] external rejects: t(Js.Promise.t('a)) => t(unit => 'a) = "rejects"; From 8fd178024f798a057e7691bd296c32227d927378 Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Tue, 16 Jul 2024 16:29:34 +0200 Subject: [PATCH 06/14] Change message on key --- ppx/reason_react_ppx.ml | 3 +-- ppx/test/key-as-prop-error.t/run.t | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml index d563916b7..fc3be7b42 100644 --- a/ppx/reason_react_ppx.ml +++ b/ppx/reason_react_ppx.ml @@ -643,8 +643,7 @@ let jsxMapper = match expr.pexp_desc with | Pexp_fun (Labelled "key", _, _, _) | Pexp_fun (Optional "key", _, _, _) -> Location.raise_errorf ~loc:expr.pexp_loc - ("Key cannot be accessed inside of a component. Don't worry - you \ - can always key a component from its parent!") + ("~key cannot be accessed from the component props. Please set the key where the component is being used.") | Pexp_fun ( ((Optional label | Labelled label) as arg), default, diff --git a/ppx/test/key-as-prop-error.t/run.t b/ppx/test/key-as-prop-error.t/run.t index 83c3c0e58..99bac040b 100644 --- a/ppx/test/key-as-prop-error.t/run.t +++ b/ppx/test/key-as-prop-error.t/run.t @@ -18,5 +18,5 @@ Test some locations in reason-react components File "component.re", line 2, characters 11-51: 2 | let make = (~key) =>
key->React.string
; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - Error: Key cannot be accessed inside of a component. Don't worry - you can always key a component from its parent! + Error: ~key cannot be accessed from the component props. Please set the key where the component is being used. [1] From e706fb275af47fd649d0c8771420a5b1ddb21f32 Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Tue, 16 Jul 2024 19:04:42 +0200 Subject: [PATCH 07/14] Fix I#843 which improves error message on react.component wrongly placed --- ppx/reason_react_ppx.ml | 8 +++---- .../component.re | 3 +++ .../components-destructured-error.t/run.t | 22 +++++++++++++++++++ 3 files changed, 28 insertions(+), 5 deletions(-) create mode 100644 ppx/test/components-destructured-error.t/component.re create mode 100644 ppx/test/components-destructured-error.t/run.t diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml index fc3be7b42..23df6bb87 100644 --- a/ppx/reason_react_ppx.ml +++ b/ppx/reason_react_ppx.ml @@ -218,14 +218,12 @@ let otherAttrsPure { attr_name = loc; _ } = loc.txt <> "react.component" let hasAttrOnBinding { pvb_attributes; _ } = find_opt hasAttr pvb_attributes <> None -(* Finds the name of the variable the binding is assigned to, otherwise raises - Invalid_argument *) +(* Finds the name of the variable the binding is assigned to, otherwise raises an error *) let getFnName binding = match binding with | { pvb_pat = { ppat_desc = Ppat_var { txt; _ }; _ }; _ } -> txt - | _ -> - raise (Invalid_argument "react.component calls cannot be destructured.") -[@@raises Invalid_argument] + | { pvb_loc; _} -> + Location.raise_errorf ~loc:pvb_loc "[@react.component] cannot be used with a destructured binding. Please use it on a `let make = ...` binding instead." let makeNewBinding binding expression newName = match binding with diff --git a/ppx/test/components-destructured-error.t/component.re b/ppx/test/components-destructured-error.t/component.re new file mode 100644 index 000000000..f737d19fc --- /dev/null +++ b/ppx/test/components-destructured-error.t/component.re @@ -0,0 +1,3 @@ +[@react.component] +let (pageState, setPageState) = React.useState(_ => 0); +let make = (~children, ()) =>
children
; diff --git a/ppx/test/components-destructured-error.t/run.t b/ppx/test/components-destructured-error.t/run.t new file mode 100644 index 000000000..e277a1a85 --- /dev/null +++ b/ppx/test/components-destructured-error.t/run.t @@ -0,0 +1,22 @@ +Test some locations in reason-react components + + $ cat >dune-project < (lang dune 3.8) + > (using melange 0.1) + > EOF + + $ cat >dune < (melange.emit + > (alias foo) + > (target foo) + > (libraries reason-react) + > (preprocess + > (pps melange.ppx reason-react-ppx))) + > EOF + + $ dune build + File "component.re", lines 1-2, characters 0-54: + 1 | [@react.component] + 2 | let (pageState, setPageState) = React.useState(_ => 0). + Error: react.component calls cannot be destructured. + [1] From d6a55d219717ca0c260298493195e0336fd5bc5b Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Tue, 16 Jul 2024 19:07:31 +0200 Subject: [PATCH 08/14] Apply same message on similar fn, update snapshot --- ppx/reason_react_ppx.ml | 5 ++--- ppx/test/components-destructured-error.t/run.t | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml index 23df6bb87..85e0756be 100644 --- a/ppx/reason_react_ppx.ml +++ b/ppx/reason_react_ppx.ml @@ -235,9 +235,8 @@ let makeNewBinding binding expression newName = pvb_expr = expression; pvb_attributes = [ merlinFocus ]; } - | _ -> - raise (Invalid_argument "react.component calls cannot be destructured.") -[@@raises Invalid_argument] + | { pvb_loc; _ } -> + Location.raise_errorf ~loc:pvb_loc "[@react.component] cannot be used with a destructured binding. Please use it on a `let make = ...` binding instead." (* Lookup the value of `props` otherwise raise Invalid_argument error *) let getPropsNameValue _acc (loc, exp) = diff --git a/ppx/test/components-destructured-error.t/run.t b/ppx/test/components-destructured-error.t/run.t index e277a1a85..492c046db 100644 --- a/ppx/test/components-destructured-error.t/run.t +++ b/ppx/test/components-destructured-error.t/run.t @@ -18,5 +18,5 @@ Test some locations in reason-react components File "component.re", lines 1-2, characters 0-54: 1 | [@react.component] 2 | let (pageState, setPageState) = React.useState(_ => 0). - Error: react.component calls cannot be destructured. + Error: [@react.component] cannot be used with a destructured binding. Please use it on a `let make = ...` binding instead. [1] From c4b1adc0f5af8d0a8a42c28dfdb7c07268896c35 Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Tue, 16 Jul 2024 19:26:19 +0200 Subject: [PATCH 09/14] Add test for record-props and record-props-error --- ppx/reason_react_ppx.ml | 19 +++++++------------ ppx/test/record-props-error.t/input.re | 6 ++++++ ppx/test/record-props-error.t/run.t | 8 ++++++++ ppx/test/record-props.t/input.re | 6 ++++++ ppx/test/record-props.t/run.t | 19 +++++++++++++++++++ 5 files changed, 46 insertions(+), 12 deletions(-) create mode 100644 ppx/test/record-props-error.t/input.re create mode 100644 ppx/test/record-props-error.t/run.t create mode 100644 ppx/test/record-props.t/input.re create mode 100644 ppx/test/record-props.t/run.t diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml index 85e0756be..a066f7c72 100644 --- a/ppx/reason_react_ppx.ml +++ b/ppx/reason_react_ppx.ml @@ -239,16 +239,13 @@ let makeNewBinding binding expression newName = Location.raise_errorf ~loc:pvb_loc "[@react.component] cannot be used with a destructured binding. Please use it on a `let make = ...` binding instead." (* Lookup the value of `props` otherwise raise Invalid_argument error *) -let getPropsNameValue _acc (loc, exp) = - match (loc, exp) with +let getPropsNameValue _acc (loc, expr) = + match (loc, expr) with | ( { txt = Lident "props"; _ }, { pexp_desc = Pexp_ident { txt = Lident str; _ }; _ } ) -> { propsName = str } - | { txt; _ }, _ -> - raise - (Invalid_argument - ("react.component only accepts props as an option, given: " - ^ Longident.last_exn txt)) + | { txt; loc }, _ -> + Location.raise_errorf ~loc "[@react.component] only accepts 'props' as a field, given: %s" (Longident.last_exn txt) [@@raises Invalid_argument] (* Lookup the `props` record or string as part of [@react.component] and store @@ -275,12 +272,10 @@ let getPropsAttr payload = } :: _rest)) -> { propsName = "props" } - | Some (PStr ({ pstr_desc = Pstr_eval (_, _); _ } :: _rest)) -> - raise - (Invalid_argument - "react.component accepts a record config with props as an options.") + | Some (PStr ({ pstr_desc = Pstr_eval (_, _); pstr_loc; _ } :: _rest)) -> + Location.raise_errorf ~loc:pstr_loc + "[@react.component] accepts a record config with 'props' as a field." | _ -> defaultProps -[@@raises Invalid_argument] (* Plucks the label, loc, and type_ from an AST node *) let pluckLabelDefaultLocType (label, default, _, _, loc, type_) = diff --git a/ppx/test/record-props-error.t/input.re b/ppx/test/record-props-error.t/input.re new file mode 100644 index 000000000..32a555110 --- /dev/null +++ b/ppx/test/record-props-error.t/input.re @@ -0,0 +1,6 @@ +module Record_props = { + [@react.component {no_props: string}] + let make = (~lola) => { +
{React.string(lola)}
; + }; +}; diff --git a/ppx/test/record-props-error.t/run.t b/ppx/test/record-props-error.t/run.t new file mode 100644 index 000000000..a8e74f7a9 --- /dev/null +++ b/ppx/test/record-props-error.t/run.t @@ -0,0 +1,8 @@ +Since we generate invalid syntax for the argument of the make fn `(Props : <>)` +We need to output ML syntax here, otherwise refmt could not parse it. + $ ../ppx.sh --output ml input.re + File "output.ml", line 4, characters 27-35: + 4 | [@@react.component { no_props = string }] + ^^^^^^^^ + Error: [@react.component] only accepts 'props' as a field, given: no_props + [1] diff --git a/ppx/test/record-props.t/input.re b/ppx/test/record-props.t/input.re new file mode 100644 index 000000000..4ad19da60 --- /dev/null +++ b/ppx/test/record-props.t/input.re @@ -0,0 +1,6 @@ +module Record_props = { + [@react.component {props: string}] + let make = (~lola) => { +
{React.string(lola)}
; + }; +}; diff --git a/ppx/test/record-props.t/run.t b/ppx/test/record-props.t/run.t new file mode 100644 index 000000000..fee6dc40a --- /dev/null +++ b/ppx/test/record-props.t/run.t @@ -0,0 +1,19 @@ +Since we generate invalid syntax for the argument of the make fn `(Props : <>)` +We need to output ML syntax here, otherwise refmt could not parse it. + $ ../ppx.sh --output ml input.re + module Record_props = + struct + external makeProps : + lola:'lola -> ?key:string -> unit -> < lola: 'lola > Js.t = "" + [@@mel.obj ] + let make = + ((fun ~lola -> + ReactDOM.jsx "div" + (((ReactDOM.domProps)[@merlin.hide ]) + ~children:(React.string lola) ())) + [@warning "-16"]) + let make = + let Output$Record_props (string : < lola: 'lola > Js.t) = + make ~lola:(string ## lola) in + Output$Record_props + end From ae983fdd36a7215ab743fb43bc28b99e1a77737e Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Tue, 16 Jul 2024 19:49:55 +0200 Subject: [PATCH 10/14] Use caller to genreate uppercase --- ppx/reason_react_ppx.ml | 4 ++-- ppx/test/create-element.t/input.re | 11 +++++++++++ ppx/test/create-element.t/run.t | 8 ++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) create mode 100644 ppx/test/create-element.t/input.re create mode 100644 ppx/test/create-element.t/run.t diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml index a066f7c72..a58f87477 100644 --- a/ppx/reason_react_ppx.ml +++ b/ppx/reason_react_ppx.ml @@ -1314,8 +1314,8 @@ let jsxMapper = (Invalid_argument "JSX: `createElement` should be preceeded by a module name.") (* Foo.createElement(~prop1=foo, ~prop2=bar, ~children=[], ()) *) - | { txt = Ldot (modulePath, ("createElement" | "make")); _ } -> - transformUppercaseCall3 ~ctxt ~caller:"make" modulePath mapper + | { txt = Ldot (modulePath, ("createElement" | "make" as caller)); _ } -> + transformUppercaseCall3 ~ctxt ~caller modulePath mapper parentExpLoc attrs callArguments (* div(~prop1=foo, ~prop2=bar, ~children=[bla], ()) *) (* turn that into diff --git a/ppx/test/create-element.t/input.re b/ppx/test/create-element.t/input.re new file mode 100644 index 000000000..28c29338c --- /dev/null +++ b/ppx/test/create-element.t/input.re @@ -0,0 +1,11 @@ +module Foo = { + [@react.component] + let createElement = (~lola) =>
{React.string(lola)}
; +}; + +module Bar = { + [@react.component] + let make = (~lola) => { + ; + }; +}; diff --git a/ppx/test/create-element.t/run.t b/ppx/test/create-element.t/run.t new file mode 100644 index 000000000..a8e74f7a9 --- /dev/null +++ b/ppx/test/create-element.t/run.t @@ -0,0 +1,8 @@ +Since we generate invalid syntax for the argument of the make fn `(Props : <>)` +We need to output ML syntax here, otherwise refmt could not parse it. + $ ../ppx.sh --output ml input.re + File "output.ml", line 4, characters 27-35: + 4 | [@@react.component { no_props = string }] + ^^^^^^^^ + Error: [@react.component] only accepts 'props' as a field, given: no_props + [1] From a23ff6739f332806b5ef63a3574ce029a3e57970 Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Tue, 16 Jul 2024 19:50:18 +0200 Subject: [PATCH 11/14] Remove Invalid_arguments --- ppx/reason_react_ppx.ml | 49 ++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml index a58f87477..fd3bb5269 100644 --- a/ppx/reason_react_ppx.ml +++ b/ppx/reason_react_ppx.ml @@ -47,8 +47,8 @@ let labelled str = Labelled str let optional str = Optional str module Binding = struct - (* Binding is the interface that the ppx uses to interact with the bindings. - Here we define the same APIs as the bindings but it generates Parsetree *) + (* Binding is the interface that the ppx relies on to interact with the react bindings. + Here we define the same APIs as the bindings but it generates Parsetree nodes *) module ReactDOM = struct let domProps ~applyLoc ~loc props = Builder.pexp_apply ~loc:applyLoc @@ -58,9 +58,6 @@ module Binding = struct end module React = struct - let null ~loc = - Builder.pexp_ident ~loc { loc; txt = Ldot (Lident "React", "null") } - let array ~loc children = Builder.pexp_apply ~loc (Builder.pexp_ident ~loc @@ -92,18 +89,22 @@ let rec find_opt p = function | [] -> None | x :: l -> if p x then Some x else find_opt p l -let getLabel str = +let getLabelOrEmpty str = match str with Optional str | Labelled str -> str | Nolabel -> "" +let getLabel str = + match str with Optional str | Labelled str -> Some str | Nolabel -> None + let optionIdent = Lident "option" let constantString ~loc str = Builder.pexp_constant ~loc (Pconst_string (str, Location.none, None)) let safeTypeFromValue valueStr = - let valueStr = getLabel valueStr in - match String.sub valueStr 0 1 with "_" -> "T" ^ valueStr | _ -> valueStr -[@@raises Invalid_argument] + match getLabel valueStr with + | Some valueStr when String.sub valueStr 0 1 = "_" -> ("T" ^ valueStr) + | Some valueStr -> valueStr + | None -> "" let keyType loc = Builder.ptyp_constr ~loc { loc; txt = Lident "string" } [] @@ -238,7 +239,7 @@ let makeNewBinding binding expression newName = | { pvb_loc; _ } -> Location.raise_errorf ~loc:pvb_loc "[@react.component] cannot be used with a destructured binding. Please use it on a `let make = ...` binding instead." -(* Lookup the value of `props` otherwise raise Invalid_argument error *) +(* Lookup the value of `props` otherwise raise errorf *) let getPropsNameValue _acc (loc, expr) = match (loc, expr) with | ( { txt = Lident "props"; _ }, @@ -246,7 +247,6 @@ let getPropsNameValue _acc (loc, expr) = { propsName = str } | { txt; loc }, _ -> Location.raise_errorf ~loc "[@react.component] only accepts 'props' as a field, given: %s" (Longident.last_exn txt) -[@@raises Invalid_argument] (* Lookup the `props` record or string as part of [@react.component] and store the name for use when rewriting *) @@ -356,7 +356,6 @@ let rec recursivelyMakeNamedArgsForExternal ~types_come_from_signature list args | _label, Some type_, _ -> type_) args) | [] -> args -[@@raises Invalid_argument] (* Build an AST node for the [@bs.obj] representing props for a component *) let makePropsValue fnName ~types_come_from_signature loc @@ -386,7 +385,6 @@ let makePropsValue fnName ~types_come_from_signature loc ]; pval_loc = loc; } -[@@raises Invalid_argument] (* Build an AST node representing an `external` with the definition of the [@bs.obj] *) @@ -399,7 +397,6 @@ let makePropsExternal fnName loc ~component_is_external (makePropsValue ~types_come_from_signature:component_is_external fnName loc namedArgListWithKeyAndRef propsType); } -[@@raises Invalid_argument] (* Build an AST node for the signature of the `external` definition *) let makePropsExternalSig fnName loc namedArgListWithKeyAndRef propsType = @@ -410,7 +407,6 @@ let makePropsExternalSig fnName loc namedArgListWithKeyAndRef propsType = (makePropsValue ~types_come_from_signature:true fnName loc namedArgListWithKeyAndRef propsType); } -[@@raises Invalid_argument] (* Build an AST node for the props name when converted to an object inside the function signature *) @@ -494,7 +490,6 @@ let makeExternalDecl fnName loc namedArgListWithKeyAndRef namedTypeList = makePropsExternal ~component_is_external:false fnName loc (List.map pluckLabelDefaultLocType namedArgListWithKeyAndRef) (makePropsType ~loc namedTypeList) -[@@raises Invalid_argument] (* TODO: some line number might still be wrong *) let jsxMapper = @@ -505,7 +500,7 @@ let jsxMapper = let argsForMake = argsWithLabels in let keyProps, otherProps = List.partition - (fun (arg_label, _) -> "key" = getLabel arg_label) + (fun (arg_label, _) -> "key" = getLabelOrEmpty arg_label) argsForMake in let jsxExpr, key, childrenProp = @@ -519,10 +514,12 @@ let jsxMapper = (label, mapper#expression ctxt expression)) in let isCap str = - let first = String.sub str 0 1 [@@raises Invalid_argument] in - let capped = String.uppercase_ascii first in - first = capped - [@@raises Invalid_argument] + match String.length str with + | 0 -> false + | _ -> + let first = String.sub str 0 1 in + let capped = String.uppercase_ascii first in + first = capped in let ident = match modulePath with @@ -584,7 +581,7 @@ let jsxMapper = let componentNameExpr = constantString ~loc:callerLoc id in let keyProps, nonChildrenProps = List.partition - (fun (arg_label, _) -> "key" = getLabel arg_label) + (fun (arg_label, _) -> "key" = getLabelOrEmpty arg_label) nonChildrenProps in @@ -682,7 +679,6 @@ let jsxMapper = "reason-react-ppx: react.component refs only support plain arguments \ and type annotations." | _ -> (list, None) - [@@raises Invalid_argument] in let argToType types (name, default, _noLabelName, _alias, loc, type_) = @@ -704,7 +700,7 @@ let jsxMapper = } ) :: types | Some type_, name, Some _default -> - ( getLabel name, + ( getLabelOrEmpty name, [], { ptyp_desc = Ptyp_constr ({ loc; txt = optionIdent }, [ type_ ]); @@ -713,7 +709,7 @@ let jsxMapper = ptyp_attributes = []; } ) :: types - | Some type_, name, _ -> (getLabel name, [], type_) :: types + | Some type_, name, _ -> (getLabelOrEmpty name, [], type_) :: types | None, Optional label, _ -> ( label, [], @@ -745,7 +741,6 @@ let jsxMapper = } ) :: types | _ -> types - [@@raises Invalid_argument] in let argToConcreteType types (name, loc, type_) = @@ -1078,7 +1073,7 @@ let jsxMapper = in let pluckArg (label, _, _, alias, loc, _) = ( label, - match getLabel label with + match getLabelOrEmpty label with | "" -> Builder.pexp_ident ~loc { txt = Lident alias; loc } | labelString -> Builder.pexp_apply ~loc From 152d64d03bf387f92a1d6cefbeb0bf3527f770b6 Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Tue, 16 Jul 2024 20:27:41 +0200 Subject: [PATCH 12/14] Add broken make_fn test and keep logic as before --- ppx/reason_react_ppx.ml | 4 +-- ppx/test/component-without-make.t/input.re | 17 +++++++++ ppx/test/component-without-make.t/run.t | 42 ++++++++++++++++++++++ ppx/test/create-element.t/run.t | 8 ----- 4 files changed, 61 insertions(+), 10 deletions(-) create mode 100644 ppx/test/component-without-make.t/input.re create mode 100644 ppx/test/component-without-make.t/run.t delete mode 100644 ppx/test/create-element.t/run.t diff --git a/ppx/reason_react_ppx.ml b/ppx/reason_react_ppx.ml index fd3bb5269..d47f95c5d 100644 --- a/ppx/reason_react_ppx.ml +++ b/ppx/reason_react_ppx.ml @@ -1309,8 +1309,8 @@ let jsxMapper = (Invalid_argument "JSX: `createElement` should be preceeded by a module name.") (* Foo.createElement(~prop1=foo, ~prop2=bar, ~children=[], ()) *) - | { txt = Ldot (modulePath, ("createElement" | "make" as caller)); _ } -> - transformUppercaseCall3 ~ctxt ~caller modulePath mapper + | { txt = Ldot (modulePath, ("createElement" | "make")); _ } -> + transformUppercaseCall3 ~ctxt ~caller:"make" modulePath mapper parentExpLoc attrs callArguments (* div(~prop1=foo, ~prop2=bar, ~children=[bla], ()) *) (* turn that into diff --git a/ppx/test/component-without-make.t/input.re b/ppx/test/component-without-make.t/input.re new file mode 100644 index 000000000..2e2e266e9 --- /dev/null +++ b/ppx/test/component-without-make.t/input.re @@ -0,0 +1,17 @@ +module Component_with_x_as_main_function = { + [@react.component] + let x = () =>
; +}; + +module Component_with_createElement_as_main_function = { + [@react.component] + let createElement = (~lola) =>
{React.string(lola)}
; +}; + +module Parent_rendering = { + [@react.component] + let make = () => { + ; + ; + }; +}; diff --git a/ppx/test/component-without-make.t/run.t b/ppx/test/component-without-make.t/run.t new file mode 100644 index 000000000..ca396def0 --- /dev/null +++ b/ppx/test/component-without-make.t/run.t @@ -0,0 +1,42 @@ +Since we generate invalid syntax for the argument of the make fn `(Props : <>)` +We need to output ML syntax here, otherwise refmt could not parse it. + $ ../ppx.sh --output ml input.re + module Component_with_x_as_main_function = + struct + external xProps : ?key:string -> unit -> < > Js.t = ""[@@mel.obj ] + let x () = ReactDOM.jsx "div" (((ReactDOM.domProps)[@merlin.hide ]) ()) + let x = + let Output$Component_with_x_as_main_function$x (Props : < > Js.t) = + x () in + Output$Component_with_x_as_main_function$x + end + module Component_with_createElement_as_main_function = + struct + external createElementProps : + lola:'lola -> ?key:string -> unit -> < lola: 'lola > Js.t = "" + [@@mel.obj ] + let createElement = + ((fun ~lola -> + ReactDOM.jsx "div" + (((ReactDOM.domProps)[@merlin.hide ]) + ~children:(React.string lola) ())) + [@warning "-16"]) + let createElement = + let Output$Component_with_createElement_as_main_function$createElement + (Props : < lola: 'lola > Js.t) = + createElement ~lola:(Props ## lola) in + Output$Component_with_createElement_as_main_function$createElement + end + module Parent_rendering = + struct + external makeProps : ?key:string -> unit -> < > Js.t = ""[@@mel.obj ] + let make () = + React.jsx Component_with_x_as_main_function.x + (Component_with_x_as_main_function.xProps ()); + React.jsx Component_with_createElement_as_main_function.make + (Component_with_createElement_as_main_function.makeProps ~lola:"lola" + ()) + let make = + let Output$Parent_rendering (Props : < > Js.t) = make () in + Output$Parent_rendering + end diff --git a/ppx/test/create-element.t/run.t b/ppx/test/create-element.t/run.t deleted file mode 100644 index a8e74f7a9..000000000 --- a/ppx/test/create-element.t/run.t +++ /dev/null @@ -1,8 +0,0 @@ -Since we generate invalid syntax for the argument of the make fn `(Props : <>)` -We need to output ML syntax here, otherwise refmt could not parse it. - $ ../ppx.sh --output ml input.re - File "output.ml", line 4, characters 27-35: - 4 | [@@react.component { no_props = string }] - ^^^^^^^^ - Error: [@react.component] only accepts 'props' as a field, given: no_props - [1] From 57eae9a4e2f00a0c127d341f65f188b5363d2da5 Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Wed, 17 Jul 2024 11:37:52 +0200 Subject: [PATCH 13/14] Install react@19 --- package-lock.json | 56 ++++++++++++++++++++++++----------------------- package.json | 4 ++-- 2 files changed, 31 insertions(+), 29 deletions(-) diff --git a/package-lock.json b/package-lock.json index 56484d8a6..fe8a6b257 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10,8 +10,8 @@ "license": "MIT", "devDependencies": { "jest": "^26.0.1", - "react": "^18.2.0", - "react-dom": "^18.2.0", + "react": "^19.0.0-beta-26f2496093-20240514", + "react-dom": "^19.0.0-beta-26f2496093-20240514", "react-test-renderer": "^18.2.0" } }, @@ -4414,49 +4414,38 @@ "dev": true }, "node_modules/react": { - "version": "18.2.0", - "resolved": "https://registry.npmjs.org/react/-/react-18.2.0.tgz", - "integrity": "sha512-/3IjMdb2L9QbBdWiW5e3P2/npwMBaU9mHCSCUzNln0ZCYbcfTsGbTJrU/kGemdH2IWmB2ioZ+zkxtmq6g09fGQ==", + "version": "19.0.0-beta-26f2496093-20240514", + "resolved": "https://registry.npmjs.org/react/-/react-19.0.0-beta-26f2496093-20240514.tgz", + "integrity": "sha512-ZsU/WjNZ6GfzMWsq2DcGjElpV9it8JmETHm9mAJuOJNhuJcWJxt8ltCJabONFRpDFq1A/DP0d0KFj9CTJVM4VA==", "dev": true, - "dependencies": { - "loose-envify": "^1.1.0" - }, "engines": { "node": ">=0.10.0" } }, "node_modules/react-dom": { - "version": "18.2.0", - "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-18.2.0.tgz", - "integrity": "sha512-6IMTriUmvsjHUjNtEDudZfuDQUoWXVxKHhlEGSk81n4YFS+r/Kl99wXiwlVXtPBtJenozv2P+hxDsw9eA7Xo6g==", + "version": "19.0.0-beta-26f2496093-20240514", + "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-19.0.0-beta-26f2496093-20240514.tgz", + "integrity": "sha512-UvQ+K1l3DFQ34LDgfFSNuUGi9EC+yfE9tS6MdpNTd5fx7qC7KLfepfC/KpxWMQZ7JfE3axD4ZO6H4cBSpAZpqw==", "dev": true, "dependencies": { - "loose-envify": "^1.1.0", - "scheduler": "^0.23.0" + "scheduler": "0.25.0-beta-26f2496093-20240514" }, "peerDependencies": { - "react": "^18.2.0" + "react": "19.0.0-beta-26f2496093-20240514" } }, + "node_modules/react-dom/node_modules/scheduler": { + "version": "0.25.0-beta-26f2496093-20240514", + "resolved": "https://registry.npmjs.org/scheduler/-/scheduler-0.25.0-beta-26f2496093-20240514.tgz", + "integrity": "sha512-vDwOytLHFnA3SW2B1lNcbO+/qKVyLCX+KLpm+tRGNDsXpyxzRgkIaYGWmX+S70AJGchUHCtuqQ50GFeFgDbXUw==", + "dev": true + }, "node_modules/react-is": { "version": "17.0.2", "resolved": "https://registry.npmjs.org/react-is/-/react-is-17.0.2.tgz", "integrity": "sha512-w2GsyukL62IJnlaff/nRegPQR94C/XXamvMWmSHRJ4y7Ts/4ocGRmTHvOs8PSE6pB3dWOrD/nueuU5sduBsQ4w==", "dev": true }, - "node_modules/react-shallow-renderer": { - "version": "16.15.0", - "resolved": "https://registry.npmjs.org/react-shallow-renderer/-/react-shallow-renderer-16.15.0.tgz", - "integrity": "sha512-oScf2FqQ9LFVQgA73vr86xl2NaOIX73rh+YFqcOp68CWj56tSfgtGKrEbyhCj0rSijyG9M1CYprTh39fBi5hzA==", - "dev": true, - "dependencies": { - "object-assign": "^4.1.1", - "react-is": "^16.12.0 || ^17.0.0 || ^18.0.0" - }, - "peerDependencies": { - "react": "^16.0.0 || ^17.0.0 || ^18.0.0" - } - }, "node_modules/react-test-renderer": { "version": "18.2.0", "resolved": "https://registry.npmjs.org/react-test-renderer/-/react-test-renderer-18.2.0.tgz", @@ -4477,6 +4466,19 @@ "integrity": "sha512-xWGDIW6x921xtzPkhiULtthJHoJvBbF3q26fzloPCK0hsvxtPVelvftw3zjbHWSkR2km9Z+4uxbDDK/6Zw9B8w==", "dev": true }, + "node_modules/react-test-renderer/node_modules/react-shallow-renderer": { + "version": "16.15.0", + "resolved": "https://registry.npmjs.org/react-shallow-renderer/-/react-shallow-renderer-16.15.0.tgz", + "integrity": "sha512-oScf2FqQ9LFVQgA73vr86xl2NaOIX73rh+YFqcOp68CWj56tSfgtGKrEbyhCj0rSijyG9M1CYprTh39fBi5hzA==", + "dev": true, + "dependencies": { + "object-assign": "^4.1.1", + "react-is": "^16.12.0 || ^17.0.0 || ^18.0.0" + }, + "peerDependencies": { + "react": "^16.0.0 || ^17.0.0 || ^18.0.0" + } + }, "node_modules/read-pkg": { "version": "5.2.0", "resolved": "https://registry.npmjs.org/read-pkg/-/read-pkg-5.2.0.tgz", diff --git a/package.json b/package.json index 63bf49935..e5eeec6de 100644 --- a/package.json +++ b/package.json @@ -26,8 +26,8 @@ "homepage": "https://reasonml.github.io/reason-react/", "devDependencies": { "jest": "^26.0.1", - "react": "^18.2.0", - "react-dom": "^18.2.0", + "react": "^19.0.0-beta-26f2496093-20240514", + "react-dom": "^19.0.0-beta-26f2496093-20240514", "react-test-renderer": "^18.2.0" }, "jest": { From 1dc29765f9754fa0993fef449e1b599df6bf9d04 Mon Sep 17 00:00:00 2001 From: David Sancho Moreno Date: Wed, 17 Jul 2024 11:38:00 +0200 Subject: [PATCH 14/14] Add Ref test --- test/Ref__test.re | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 test/Ref__test.re diff --git a/test/Ref__test.re b/test/Ref__test.re new file mode 100644 index 000000000..551e198ed --- /dev/null +++ b/test/Ref__test.re @@ -0,0 +1,43 @@ +open Jest; +open Jest.Expect; +open ReactDOMTestUtils; +open Belt; + +/* https://react.dev/blog/2022/03/08/react-18-upgrade-guide#configuring-your-testing-environment */ +[%%mel.raw "globalThis.IS_REACT_ACT_ENVIRONMENT = true"]; + +module Button_with_ref = { + [@react.component] + let make = (~children, ~ref) => { + ; + }; +}; + +describe("React - Ref", () => { + let container = ref(None); + + beforeEach(prepareContainer(container)); + afterEach(cleanupContainer(container)); + + test("can render DOM elements", () => { + let container = getContainer(container); + let root = ReactDOM.Client.createRoot(container); + let domRef = React.createRef(); + + act(() => { + ReactDOM.Client.render( + root, + + "Click me"->React.string + , + ) + }); + + expect( + container + ->DOM.findBySelectorAndTextContent("button", "Click me") + ->Option.isSome, + ) + ->toBe(true); + }); +});