ml-in-barcelona / jsoo-react

js_of_ocaml bindings for ReactJS. Based on ReasonReact.
https://ml-in-barcelona.github.io/jsoo-react
MIT License
138 stars 19 forks source link

Fix external%component to not make assumptions about the JS component type #123

Closed jchavarri closed 2 years ago

jchavarri commented 2 years ago

Reported by @glennsl in https://github.com/ml-in-barcelona/jsoo-react/pull/106#issuecomment-1021597956.

Current code generated by ppx for external%component adds a make function with arity 1 that just makes a function call to the JS component and pass props. But JS components can be objects too (forwardRef, class components and such).

Ppx should not make any assumptions about JS components.

We might work around this by making that the ppx output code something like this:

-let make
-  (Props :
-    < name: Js.js_string Js.t Js_of_ocaml.Js.readonly_prop   > 
-      Js_of_ocaml.Js.t)
-  =
-  (((Js_of_ocaml.Js.Unsafe.js_expr
-        "require(\"my-react-library\").MyReactComponent") Props)
-  [@warning "-20"]) in
fun ~name ->
-  fun ?key -> fun () -> React.createElement make (make_props ?key ~name ())
+  let t ?key () =
+    React.createElement
+      (Js_of_ocaml.Js.Unsafe.js_expr "require(\"my-react-library\").MyReactComponent" )
+      (make_props ?key ~name ())

So we just remove any of the assumptions and let React handle the component itself, directly.

glennsl commented 2 years ago

Gave it a shot here: https://github.com/ml-in-barcelona/jsoo-react/compare/main...glennsl:fix/external/children

Still get the same error, using forwardRef to create a non-function component :disappointed:

Test baseSuite:externals:non-function ... Error: Error: Objects are not valid as a React child (found: object with keys {$$typeof, render}). If you meant to render a collection of children, use an array instead.
    in NonFunctionGreeting (took 0.0010s)
glennsl commented 2 years ago

Also added a test for passing children, which works but is a bit awkward to use, especially for Reason since JSX makes children a list, and external components have to be passed a Js.js_array.

jchavarri commented 2 years ago

Sorry, i don't have time now to look deep into this, but from quick check, wouldn't the forwardRef component in external.js need to take props in?

const NonFunctionGreeting = React.forwardRef(({name}, _ref) => React.createElement("span", null, "Hey ", name));
glennsl commented 2 years ago

Ah, of course! Thanks! I meant to wrap Greeting as a whole, not just the body of it, but somewhere along the line I guess my brain had a meltdown.

Anyway, that works! I'm sure this could be simplified further, but I'll create a PR for it, then perhaps you could do another pass on it?