Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Experimental suspense+cm router #217

Closed
zth opened this issue May 29, 2021 · 12 comments
Closed

[RFC] Experimental suspense+cm router #217

zth opened this issue May 29, 2021 · 12 comments

Comments

@zth
Copy link
Owner

zth commented May 29, 2021

As detailed here: https://rescript-relay-documentation.vercel.app/docs/experimental-router

Please add any thoughts, comments or feedback about the router to this issue.

@mellson
Copy link
Contributor

mellson commented May 31, 2021

Should renderPending be rewritten to only be used when it's pending. Right now it's used something like this:

<RescriptRelayRouter.RouteRenderer
  renderFallback={_ => <LoadingScreen />}
  renderNotFound={_ => <NotFound />}
  renderPending={pending =>
    switch pending {
    | true => <PendingElement />
    | false => React.null
    }}
/>

Is it a normal usecase to render an element when pending is false?

I think it would be simpler like this:

<RescriptRelayRouter.RouteRenderer
  fallback={<LoadingScreen />}
  notFound={<NotFound />}
  pending={<PendingElement />}
/>

@zth
Copy link
Owner Author

zth commented May 31, 2021

@mellson yeah, that's a good point! The reason it's designed the way it is is to make it easy to do enter/exit animations on the indicator (keeping one element rendered so you can do regular CSS transitions etc). But it's definitively worth considering!

@mellson
Copy link
Contributor

mellson commented May 31, 2021

Is it necessary to split up matchUrl and render ?
If everything was handled in render we wouldn't need our own types for routeparams.

Sorry, forget that. I was too fast. It's needed for prepare as well 👍🏻

@mellson
Copy link
Contributor

mellson commented May 31, 2021

@zth that makes sense. I think keep it as it is. I was just a little confused because the renderPending bool wasn't used in the current docs. I tried to make it more explicit in the PR on the docs.

@zth
Copy link
Owner Author

zth commented May 31, 2021

Is it necessary to split up matchUrl and render ? If everything was handled in render we wouldn't need our own types for routeparams.

Sorry, forget that. I was too fast. It's needed for prepare as well 👍🏻

Yes, the rationale for splitting in 3 steps is that each step is needed, in the sense that:

  1. We need to be able to check whether a route family matches, without necessarily doing anything.
  2. We need to be able to call prepare independently of render, to be able to preload a route and its dependencies without necessarily rendering it. At a later stage, this will also be key for SSR - we'll hopefully be able to leverage prepare to prepare (and wait for the route to be fully prepared) a route before rendering it server side.
  3. Finally, obviously we need to be able to render.

The alternative would be to merge matchUrl and prepare, but that would mean that we loose the ability to check whether URLs match (and extract params) independently without side effects like loading queries etc.

This is a very interesting discussion and the thing I've wrestled with the most when working through the current approach. Very happy to discuss it more, weigh pros and cons etc.

@mellson
Copy link
Contributor

mellson commented May 31, 2021

Should the router maybe provide a wrapper around the RescriptReactRouter methods for push and others? It's a bit confusing using two things with so similar names 😆

It would be nice to be able to do one or both of these:

  1. RescriptRelayRouter.push(url)
  2. router.push(url) (after importing it let router = RescriptRelayRouter.use()

@zth
Copy link
Owner Author

zth commented Jun 1, 2021

Yes, that's a great idea! We should definitely have that. Let me set that up and update the gists. Actually, I'll move the code into the repo (but not expose it in the package for now).

Is there anything else around convenience you've thought of?

@mellson
Copy link
Contributor

mellson commented Jun 1, 2021

@zth that sounds great. I don't have any more inconveniences right now. I think it's a great product you're building! I've played around with preloading data and code, and everything works smoothly. It's so nice, a huge upgrade over the included router when using Relay.

@zth
Copy link
Owner Author

zth commented Jun 6, 2021

The code now exists here: https://github.com/zth/rescript-relay/tree/master/packages/rescript-relay/src/experimental-router

It also has a useRouter hook now with shortcuts for navigating and preloading:

https://github.com/zth/rescript-relay/blob/master/packages/rescript-relay/src/experimental-router/RescriptRelayRouter.resi#L28-L36

@mellson
Copy link
Contributor

mellson commented Jun 7, 2021

@zth nice, love the addition of useRouter 👍🏻

Should that hook include RescriptReactRouter.useUrl() and the other hooks, or what's your thinking on these?

@zth
Copy link
Owner Author

zth commented Jun 7, 2021

I haven't thought about it too much yet, but ideally I would want to split up "static" stuff/actions and things like useUrl anyway, so it's easier to control re-renders. So, maybe is the answer, I guess 😄

@zth
Copy link
Owner Author

zth commented Aug 1, 2024

Done in https://github.com/zth/rescript-relay-router/blob/main/packages/rescript-relay-router/README.md

@zth zth closed this as completed Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants