tom-sherman / rescript-remix

MIT License
30 stars 5 forks source link

Support `CatchBoundary` and `ErrorBoundary` #18

Open drewschrauf opened 2 years ago

drewschrauf commented 2 years ago

The convention based error handling in Remix requires exporting React components with the names CatchBoundary and ErrorBoundary. As Rescript requires that all values start with a lowercase letter, we need to do some work to support it.

I was initially thinking we could just use a raw code block:

@react.component
let errorBoundary = () => {}
%%raw(`exports.ErrorBoundary = errorBoundary`)

However, it seems that Remix requires the use of es6 modules and that strategy only works for commonjs modules. With es6 modules, Rescript produces an export {} block at the bottom of the file and it doesn't seem possible to change that with a workaround like this.

I did some more research and it appears that we can make use of genType to cover this requirement. This would allow us to annotate the boundary components like so:

@react.component @genType.as("ErrorBoundary")
let errorBoundary = () => {}

This isn't without its baggage though:

The first two points only affect the template's bsconfig.json. Point 3 would require some work in this repo.

Does this seem like the right path forward? Did you have other thoughts about how this might be accomplished?

tom-sherman commented 2 years ago

I don't think any additional work needs to be done here except maybe some documentation, I was under the impression that

module ErrorBoundary = {
  @react.component
  let make = () => React.null
}

Would export a component named ErrorBoundary.

tom-sherman commented 2 years ago

Ah, that adds an additional ErrorBoundary.make thingy. How frustrating.

function Foo$ErrorBoundary(Props) {
  return null;
}

var ErrorBoundary = {
  make: Foo$ErrorBoundary
};

export {
  ErrorBoundary , 
}
tom-sherman commented 2 years ago

However, it seems that Remix requires the use of es6 modules and that strategy only works for commonjs modules.

Does it? I would expect this to work also

%%raw(`export const ErrorBoundary = errorBoundary`)
drewschrauf commented 2 years ago

However, it seems that Remix requires the use of es6 modules and that strategy only works for commonjs modules.

Does it? I would expect this to work also

%%raw(`export const ErrorBoundary = errorBoundary`)

You're right, that does work. I had tried that previously with no luck... must have had a stale build or did something dumb.

I think genType is the "correct" solution to the problem (from a Rescript perspective) but it comes with its own baggage that feels a little too high-friction for a convention-based system like this. I think this problem can probably be solved with docs and an example or two in the template repo.

drewschrauf commented 2 years ago

Ok, I figured out what my issue was. When you're using the @react.component annotation, your variables are created in JS with a different name. So:

let catchBoundary = () => "Oops"->React.string
%%raw(`export const CatchBoundary = catchBoundary`)

@react.component
let errorBoundary = () => "Uh oh"->React.string
%%raw(`export const ErrorBoundary = errorBoundary`)

produces the JS:

function catchBoundary(param) {
  return "Oops";
}

export const CatchBoundary = catchBoundary
;

function Route_index$errorBoundary(Props) {
  return "Uh oh";
}

export const ErrorBoundary = errorBoundary
;

var errorBoundary = Route_index$errorBoundary;

In the above, the CatchBoundary is exported correctly but the ErrorBoundary isn't. I don't really know how old-school stuff like variable hoisting interacts with new-school stuff like export 😅 It's certainly not doing what we want though.

This is solved by using the generated name (i.e. Route_index$errorBoundary) in the raw export, or by skipping the @react.component annotation. You only need the annotation when you're using props so it's fine for the CatchBoundary but the ErrorBoundary takes a single prop called error. You can, again, work around that by using a single positional argument for the ErrorBoundary but it's a little goofy.

I'm finding a few rough edges while implementing the jokes app. It'll be interesting to look through the PR for that when I'm done and see where the gaps are.