divviup / divviup-api

Divvi Up Control Plane
https://divviup.org
Mozilla Public License 2.0
4 stars 1 forks source link

consider switching routers from react-router to tanstack's router #262

Open jbr opened 1 year ago

jbr commented 1 year ago

Most of the worst code in the app relates to the router (eg #261), which continues to disappoint. As far as I can tell, react-router is moving in the direction of primarily supporting remix's ssr solution and the api has suffered as a result. Additionally, the type declarations are the weakest part of the app. I have no experience with tanstack's router, but it can't be worse.

divergentdave commented 9 months ago

Having read up on React and React Router, I can see how it would cause issues.

Since it has its own Form component, (two, actually) it must take extra work to integrate with formik.

The loader/action features are expected to be used together, in both directions of data flow to/from the server, which makes me think React Router is halfway to being a framework. I'm uneasy about the complicated logic determining which useLoaderData() calls get invalidated on a navigate/fetcher call. The tutorial claims that loaders for "unchanging" routes like the contacts list in its example do not get called during navigation. That sounds like it never updates the list of contacts, but that would be incorrect if the user just created a new contact. It's not clear whether or how you could override this decision and invalidate that part of loader data, but this would add more complication and more edge cases to test. On the other hand, for routes that are not "unchanging", having useLoaderData() take care of invalidation seems like it could be too big a hammer at times, there will probably be cases where only select parts of data needs to be re-fetched.

The tutorial recommended against using controlled components to keep UI contents in sync with navigation, and instead suggests combining a reactive defaultValue attribute and a bare useEffect() to take care of transitions that the former misses. React Router seems to be encouraging an error-prone coding style.

I was surprised that elements for nested routes need a <Outlet> placeholder, it seems that a more natural approach would be to take a component, and pass a children prop.