rescript-react icon indicating copy to clipboard operation
rescript-react copied to clipboard

Fix ErrorBoundary component using external binding

Open mununki opened this issue 2 years ago • 3 comments

I found the critical error during the thorough test for V10.

Warning: Functions are not valid as a React child. This may happen if you return a Component instead of <Component /> from render. Or maybe you meant to call this function rather than return it.
    at RescriptReactErrorBoundary

This warning is not caused by V10, however, the generated js representation needs to be fit to React API. I tried to implement the ErrorBoundary component by using jsx ppx, but no gain so far. I think I should revert it and look later.

mununki avatar Jul 24 '22 08:07 mununki

The generated js representation should be

var make = getErrorBoundary(React.Component);

But, by using jsx ppx

function RescriptReactErrorBoundary(Props) {
  return getErrorBoundary(React.Component);
}

var make = RescriptReactErrorBoundary;

mununki avatar Jul 24 '22 09:07 mununki

Would you create a reminder issue to revisit this later?

cristianoc avatar Jul 24 '22 09:07 cristianoc

~~I found one solution to make the ErrorBoundary component works by using jsx ppx. By using the jsx ppx in normal way, the generated js output is inevitable.~~

function RescriptReactErrorBoundary(Props) {
  return getErrorBoundary(React.Component);
}

var make = RescriptReactErrorBoundary;

~~But, there is another way to use the jsx ppx, external~~

@react.component @module("./react/ErrorBoundary.js")
external make: (
  ~children: React.element,
  ~fallback: params<'error> => React.element,
) => React.element = "default"

// generated
var ErrorBoundaryJs = require("./react/ErrorBoundary.js").default;

var make = ErrorBoundaryJs;

~~And It works just fine.~~

mununki avatar Jul 24 '22 17:07 mununki

I fixed the runtime error of ErrorBoundary. It enables us to use @react.component without needing external binding to the ErrorBoundary component in javascript.

mununki avatar Sep 01 '22 05:09 mununki

Need someone to double-check, or are you confident that it's right and can be merged?

cristianoc avatar Sep 01 '22 05:09 cristianoc

Need someone to double-check, or are you confident that it's right and can be merged?

I'm confident that it fixes the runtime error caused by #47

mununki avatar Sep 01 '22 05:09 mununki