tom-sherman / rescript-remix

MIT License
30 stars 5 forks source link

Support convention-based routing #4

Closed drewschrauf closed 2 years ago

drewschrauf commented 2 years ago

Great work on this repo so far! I was looking to write my own ReScript bindings then stumbled on this project.

Issue

There are a few issues that mean Remix's convention-based routing won't work with ReScript. In particular:

Assuming that convention-based routing is within the scope of this project (it seems to be used currently), I think we need an alternative set of conventions for ReScript projects.

Proposal

It's possible to leverage the routes value of remix.config.js to map any arbitrary file-system representation into a set of routes. I propose that we introduce our own routing conventions for ReScript that are inspired by standard Remix but are more compatible with ReScript.

Overall, the jokes tutorial app would have the following files:

app
├── entry.client.res
├── entry.server.res
├── reroutes
│  ├── index.res
│  ├── jokes
│  │  ├── jokes__[jokeId].res
│  │  ├── jokes__index.res
│  │  └── jokes__new.res
│  ├── jokes.res
│  ├── jokes[[.]]rss.res
│  ├── login.res
│  └── logout.res
└── root.res

I'm working on a PR with this logic but wanted to make sure it aligned with your thoughts/goals for this project before opening it.

tom-sherman commented 2 years ago

Thanks for taking the time to write this up! I've been thinking a bit about this problem, have some discussion over on the forum about it: https://forum.rescript-lang.org/t/remix-folder-structure-and-rescript-modules/2817

I'll take a look at your proposal ASAP 👍

tom-sherman commented 2 years ago

This looks perfect! I have one question:

Should the custom routing take into account directory structure (a la Remix) or ignore it and just go off the module name (a la ReScript)? I see pros and cons of both approaches.

Directory structure approach:

Module name only approach:

So in summary I can see three approaches:

  1. Ignore directory structure, use only module name conventions
  2. Use directory structure only, ignore namespace__ prefixes
  3. Use directory structure only, enforce that namespace__ prefixes match the directory structure

I'm assuming no. 3 is possible and that throwing errors inside the routes function will fail the build.

I'm leaning on no. 2 personally as I think it will offer the best DX for deeply nested routes, while making sense to both ReScript and Remix developers. It encodes a simple rule for file based routing: "Remix routing but you need to add a unique namespace__ prefix to satisfy the ReScript compiler"


A different point, if you care to take a step into my bikshed for a moment 😅

I'd prefer a different name to reroute for the folder. "Reroute" is a relatively common verb that I've seen people use in place of "redirect", whereas here the "re" stands for ReScript. Maybe resroutes, res-routes, or src-routes?

drewschrauf commented 2 years ago

I just had a read of that forum thread. I really like the ~ convention! bobzhang also had a suggestion of adding an argument to the no_export directive to rewrite the filename. That also seems to be a general solution to the problem but I think you still need unique filenames for compilation.

I may be coming at this problem from more of a "workaround" angle than a "solution" angle.

drewschrauf commented 2 years ago

Thanks for the feedback!

Interesting thought on ignoring the directory structure entirely and just building the routes directly from the file names. I feel that it might blur the line between dot-delimited paths and the hierarchy implied by the namespaces. I think it's particularly tricky because the Remix directory structure is simultaneously attempting to declare the route hierarchy and the view hierarchy. It could be simpler than the folder approach though... I'll need to think on it.

My thinking is that the ideal solution to this problem would be to use the original Remix directory structure with your magic ~ prefix. With that in mind, I'm inclined to follow the Remix structure as closely as possible to a) not require users to learn a totally different naming scheme; and b) make for an easier migration in the future if/when ReScript allows raw file names. The ceremony around namespace__ prefixing sucks but I'm inclined to treat it as a necessary (and hopefully temporary) evil rather than leaning into it with options 1 and 3. As you said, "Remix routing but you need to add a unique namespace__ prefix to satisfy the ReScript compiler".

I'd prefer a different name to reroute for the folder. "Reroute" is a relatively common verb that I've seen people use in place of "redirect", whereas here the "re" stands for ReScript. Maybe resroutes, res-routes, or src-routes?

😂💯 My default of making everything re* is a bit of a holdover from the ReasonML days. I'm going to run with res-routes for now. It's easy to change once the PR is open if we think of something better.


As an aside, the parsing logic would be a million times easier to write with tests than doing it manually but adding tests for template utilities seems like a bit of a faux pas. Not to mention, it seems that Jest isn't a particularly good fit for Remix testing. I think KCD is using cypress for his own stuff at the moment. At this point I'm considering just making a scratch project off to the side with Jest, writing the logic there, and copying the resulting method back into this project. Thoughts?

tom-sherman commented 2 years ago

Option 1 I was mostly mentioning for completeness, I do think leaning into Remix conventions is better here.

At this point I'm considering just making a scratch project off to the side with Jest, writing the logic there, and copying the resulting method back into this project.

I think at this point it may be worth publishing a package to npm that includes the bindings and the routing helper (which could even be written in ReScript). That package could then be added as a dependency of this template.

I think this has interesting applications for gradual adoption too which would be really cool.

drewschrauf commented 2 years ago

I think at this point it may be worth publishing a package to npm that includes the bindings and the routing helper (which could even be written in ReScript). That package could then be added as a dependency of this template.

Sounds like a plan. I'm happy to do it but don't want to step on your toes here. I'm implementing the routes function in a separate project with @glennsl/rescript-jest and mock-fs for now. I can open a PR against the new npm module when that exists (or can set that up myself if you want).

tom-sherman commented 2 years ago

@drewschrauf I've created the repo: https://github.com/tom-sherman/rescript-remix

Currently trying to figure out @rescript/react peer dependency stuff but feel free to raise a PR there 😄

drewschrauf commented 2 years ago

😂 Peer dependencies, the bane of my existence. I'm building things out here if you're interested in a peek. I'll fork the repo and open a proper PR once I've got it all working.

drewschrauf commented 2 years ago

I was trying to test my code before opening a PR and haven't been able to get rescript-remix to work with rescript-remix-template. I've tried using npm link and npm pack/npm install but I keep seeing errors to the effect of:

FAILED: app/root.cmj
We've found a bug for you!
/home/drew/Code/rescript-remix-template/app/root.res:12:8-17
10 ┆   <meta charSet="utf-8" />
11 ┆   <meta name="viewport" content="width=device-width,initial-scale=1" />
12 ┆   <Remix.Meta />
13 ┆   <Remix.Links />
14 ┆ </head>
The value makeProps can't be found in Remix.Meta
FAILED: cannot make progress due to previous errors.

Have you been seeing the same issues? I'm not sure if I'm being dumb or if there's a legit issue with this module.

tom-sherman commented 2 years ago

Yeah these are issues I'm receiving too 😕 it's what I was referring to with peer dependency issues, could you try installing from npm?

I'm going to take another look tomorrow, I must have something setup incorrectly.

See https://forum.rescript-lang.org/t/how-to-do-peer-dependencies-when-publishing-a-rescript-npm-package/2837