tom-sherman / rescript-remix

MIT License
30 stars 5 forks source link

Add example app: jokes #20

Closed drewschrauf closed 1 year ago

drewschrauf commented 2 years ago

I've put together a port of the jokes app. It's basically all there except the the optimistic UI when creating a new joke. I'm opening this PR as a draft cause I think there are a few issue that are worth figuring out before telling everyone "this is how you do it".

Type safety

You've already done some thinking about this with regards to data loading and KCD opened an issue on the Typescript repo to address some related issues. It's hard to unravel them all to separate talking points but I'll do my best!

Enforcing correct types on route module exports - Route modules have to export the correct functions of the correct types. I feel like this may just be an unavoidable issue when it comes to convention based frameworks and is the same problem that KCD opened the issue for. Rescript has *.resi files but how do you ensure that they're using the correct types?

There's an option to automatically generate the *.resi files using the js-post-build hook. That has its own issues though and feels a little heavy handed.

Enforcing correct types on loader/action return types - This is the issue you were playing with. The problem is that Remix's return type on loader/action functions is (basically) Data | Promise<Data> | Response | Promise<Response>. It would be pretty easy to declare a standard function signature that returned Promise<Data> but it turns out that the Response versions are used constantly. Within the jokes app, they're used often for redirects after form submissions and augmenting standard data responses with additional headers (Vary, for example). The reliance on web standards is a bit of a curse here as Response is clearly not typed.

Furthermore, these data types are actually used constantly in remix. For example, the meta function gives the user access to the loaderData to customise the headers. useMatches gives the user access to not only the current route's data but all parent routes as well. This is generally typed as any in the Typescript bindings, I assume due to the complexity.

I think there's a few options here:

Server modules on the client

This is a bit of a limitation of Rescript again, similar to the convention-based routing issues we encountered previously. Basically, route modules are a single file containing isomorphic code, as well as code that will only ever run on the server. During development, I think Remix serves up the route modules as-is, meaning that DB dependencies will get bundled for the client. This is a problem when these modules rely on native code that can't get bundled (I ran into issues with bcrypt during the port, opting to use bcryptjs in its place). I'm pretty sure this isn't an issue during production builds as tree-shaking should remove methods like loader that would have interacted with the DB.

I'm pretty sure this is why Remix's naming convention of db.server.js exists. I think this ensures that the db module won't get bundled for the client under any circumstances. Unfortunately, using . in a Rescript module name makes it exotic and impossible to import from another module, making db.server.res kind of useless.

I don't think we have any other recourse here except for a change to Remix itself to allow an alternate naming strategy for client/server modules. I haven't looked at whether this is possible, let alone welcome, but it's definitely an issue.


I'm keen to hear your thoughts on this!

drewschrauf commented 2 years ago

I've been playing with another branch (branched from this PR branch) where I've:

The ergonomics around using Js.Json.t actually feels much nicer and seems to align with Remix's goal of "use the platform". It's almost like replacing Remix's use of any with something more analogous to unknown that requires runtime checks. It also doesn't require making any custom Remix-focused bindings for the Webapi module that try and enforce type safety (such as a Remix.Response.t<'appData>). Obviously this is all at the cost of some compile-time type safety (you're not forcing that loader returns the same type that's read by useLoaderData) but the run-time safety should make sure your code will either run with good data or throw with bad data (instead of running with bad data).

tom-sherman commented 2 years ago

@drewschrauf amazing, really appreciate your help here! Sorry I haven't got round to these yet, caught up in a bunch of things atm 😅

Will try to take a look this week 🙇

drewschrauf commented 2 years ago

No worries! I've been tinkering a little less since the cricket season ended anyway 😂