React.forwardRef is unsafe

[alex35mil]

Usecase:

module X = {
  [@react.component]
  let make = React.forwardRef(theRef =>
    <div ref=?{theRef->Js.Nullable.toOption->Belt.Option.map(ReactDOMRe.Ref.domRef)} />
  );
};

[@react.component]
let make = () => <X ref={theRef => theRef->React.Ref.current->Js.log} />;

This compiles b/c theRef type is inferred in the callback, but it crashes at runtime since in this case theRef is of type Js.nullable(Dom.element).

[jchavarri]

I investigated this a bit as part of the work in rroo.

The ref exposed by forwardRef is of type 'a, so it can be matched with anything really. If X is declared like above, this type checks as well:

[@react.component]
let make = () => <X ref=12345 />;

I believe one fix could be:

1. Simplify the types in forwardRef, by using ReactDOMRe.domRef type. This doesn't fix anything but I think it'd help avoid issues in the future if some of these types change. 😄

[@bs.module "react"]
external forwardRef:
  ([@bs.uncurry] (('props, ReactDOMRe.domRef) => React.element)) =>
  React.component('props) =
  "";

2. Make sure the JSX ppx doesn't generate a type variable for ref and instead refine it by making it of type ReactDOMRe.domRef. I believe the ppx code doesn't live in this repo anymore and it's part of the jscomp/syntax in BuckleScript repo. The line to be changed is this:

   https://github.com/BuckleScript/bucklescript/blob/89efbef61090b41e083387802f74a49879bff344/jscomp/syntax/reactjs_jsx_ppx.cppo.ml#L772

   Instead of the None at the end of the tuple, it should be Some(domRefDef) where domRefDef is an ast type core_type that represents ReactDOMRe.domRef (example in astexplorer).

If anyone wants to explore how these changes would look like, here's a version of X above with the changes applied, unppxed version included for clarity:

[@bs.module "react"]
external forwardRef:
  ([@bs.uncurry] (('props, ReactDOMRe.domRef) => React.element)) =>
  React.component('props) =
  "";

module X = {
  [@react.component]
  let make =
    forwardRef(theRef => {
      <div ref=theRef> "ForwardRef"->React.string </div>;
    });
};

// Would compile to:

module X = {
  [@bs.obj]
  external makeProps:
    (~key: string=?, ~ref: ReactDOMRe.domRef=?, unit) => Js.t({.}) =
    "";
  let make =
    forwardRef(
      {
        let test = (_props: Js.t({.}), theRef) => {
          ReactDOMRe.createDOMElementVariadic(
            "div",
            ~props=ReactDOMRe.domProps(~ref=theRef, ()),
            [|"ForwardRef"->React.string|],
          );
        };
        test;
      },
    );
};

Then calling

<X ref={theRef => theRef->React.Ref.current->Js.log} />

would fail with:

Error: This expression should not be a function, the expected type is ReactDOMRe.domRef

[jchavarri]

Here's the related PR in rroo, in case it helps: ml-in-barcelona/jsoo-react#19. The required changes in the ppx are tiny: https://github.com/jchavarri/rroo/pull/19/files#diff-ec2648e0bf16200f53280d61f4d4ee33.

One thing to note is that I had to move forwardRef from React to ReactDOM module to avoid cyclic deps.

[peterpme]

Hey folks,

For the sake of cleaning up the repo (and given how old this issue is) I'm going to close this out.

Thank you and sorry about the delay. Please re-open if its still relevant!

[davesnx]

Re-opening since this might still be relevant