Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Make Routes component participate in data loading #9254

Closed
MrHus opened this issue Sep 14, 2022 · 5 comments
Closed

Make Routes component participate in data loading #9254

MrHus opened this issue Sep 14, 2022 · 5 comments
Labels

Comments

@MrHus
Copy link

MrHus commented Sep 14, 2022

What is the new or updated feature that you are suggesting?

Make the <Routes> component participate in data loading.

Why should this feature be included?

Currently I'm using the <Routes /> component as a way to split up my routes in multiple different files.
This way my routes do not grow very large, and I keep things manageable.

Currently I'm looking at the new "loader" feature and how to integrate it in my application. But the loaders
do not work for nested <Routes />. The documentation says:

"If you're using a data router like createBrowserRouter it is uncommon to use this component as it does not participate in data loading."

It would be nice to support <Routes /> in combination with the "createBrowserRouter" as to not get a GOD routing
file containing all routes of a large application.

The only way forward I see now using the current API is not using "createRoutesFromElements" and importing
"arrays" and merge them at the top, but this way I loose the "routes as jsx" features.

@MrHus MrHus added the feature label Sep 14, 2022
@maxprilutskiy
Copy link

maxprilutskiy commented Sep 16, 2022

I second this.

Unless I'm missing something, to use the new loader feature, createBrowserRouter suggests defining all the routes upfront and using that together with <Outlet />s.

However, if you think about it, it just undermines code-splitting techniques. Using routes-at-the-root solution would require either reference route components directly, which would disable code-splitting, or lazy loading routes as well as all of their child routes to make it work. For example, it would be simply impossible to make /users and /posts load lazily, while /user/:userId and /post/:postId in a regular way together with their respective parent routes.

When a component is required to know about not only its direct children, but also about children of its children, it just breaks incapsulation and becomes an unlikely scalable solution.

I understand react-router team might push the community towards using createBrowserRouter and routes-at-the-root approach to make it easier then to switch to remix in the future. And that could be really nice to have this option some day. But if this API breaks such a crucial thing as defined by the user code splitting, it just simply would not work.

Alternatively, what I thought of is that we could just start nesting prefixed RouterProviders throughout the app, to avoid defining all the routes at the root of the app; but I haven't tested this hypothesis, and it doesn't sound quite right.

@brophdawg11
Copy link
Contributor

The only way forward I see now using the current API is not using "createRoutesFromElements" and importing
"arrays" and merge them at the top, but this way I loose the "routes as jsx" features.

@MrHus createRoutesFromElements simply transforms the JSX to the array notation - so there shouldn't be any reason you couldn't leverage that to import your sub-arrays (written in JSX) and merge them at the top?

@maxprilutskiy We will likely eventually add some "recommended" code splitting approaches to the docs, but you have various options available in the current setup. Another community member did a great write-up of some options here: https://dev.to/infoxicator/data-routers-code-splitting-23no.

As for making <Routes> participate in data loading, I don't think we've ruled it out completely, but it goes a bit against the ideas behind 6.4 - which is to decouple data fetching from rendering. Fetching inside the render lifecycle (i.e., when rendering descendant <Routes>) re-introduces the same waterfalls/spinners we're hoping the 6.4 APIs eliminate! Ryan gave a good talk on this at Reactathon: https://www.youtube.com/watch?v=95B8mnhzoCM.

Here's the main challenge with getting descendant Routes to participate in data-loading, as I see it:

  1. Assume your app defines <Route path="/users/*" loader={userLoader} element={<UsersApp/>}> in which UsersApp has descendant <Routes>
  2. Your user clicks <Link to="/users/1">
  3. At this point, all the router knows about is the /users/* route, so we go into a navigation.state="loading" state and run the singular loader we know about ahead of the render pass.
  4. Loading finishes and we update state.location="/users/1" and navigation.state="idle" and hand off to the render cycle.
  5. Uh-oh! During render we just found more routes with loaders - so we can't actually render since we've not yet got all of our data!
    1. What do we render? We don't have data so anything calling useLoaderData() inside your descendant Routes is going to break :/. So you need to now do data-checking inside your components and this breaks the happy-path guarantee of useLoaderData, and it becomes the same DX as if we were just fetching in components in 6.3 and before.
    2. Do we throw back out to a higher level suspense boundary? That breaks the current UI you were on prior to clicking the link though :/
    3. Do we go back into navigation.state=loading? That re-triggers the global loading indicators that were just removed because loading finished :/

So the idea of allowing descendant Routes (unknown to the router prior to render time) is a bit at odds with some of the major design goals of 6.4 (decouple fetching and rendering, reducing spinners, etc.)

We do agree that it's currently non-optimal for large apps to have to specify all of their routes up-front, and we have ideas in mind to alleviate that by adding some form of runtime route discovery, but I don't think that discovery will necessarily happen in the react render lifecycle - more likely it will happen through some other API we can tap into prior to rendering.

I'm going to leave this open for now but I suspect this will get converted into a Discussion in which we can continue to figure out the right API for the type of route discovery

@gaspardip
Copy link

gaspardip commented Oct 13, 2022

So I wrote this on the remix discord a while ago but I'll leave it here so it doesn't get lost.

The current solution makes too many assumptions about the application and takes away too much control away from users in my opinion.

Kind of related to what @maxprilutskiy said, I think a good solution would be being able to declare multiple "sub" routers, then have a "root" router that composes all of them, very similar to what express.Router does, where you are able to declare a router for an entire route tree and apply middlewares and whatnot to it.

Why this is good:

  • It allows me to be as granular as I want with my route definitions (giving users back some control about where they want their routes to live in) + allows me to keep route encapsulation
  • It allows for more natural code-splitting (the current workaround I was pointed to feels like a hack to me)
  • It still decouples data-fetching from rendering (spirit of 6.4) and does so outside the react's render cycle (vs runtime discovering/registering)
  • Compatible with current requirement for data-loading (declaring all routes at the top level). You could still technically declare all of them in one go, but in case you want to be more granular, you'd be able to.

Granted, the "root" router wouldn't be able statically determine the entire route tree but that doesn't matter because each "sub-router" would know the routes it cares about and the data they need to fetch in order to be rendered.

Here's a very rough usage as I imagine it:

// users/routes.tsx
import { UserLayout } from './UserLayout';
import { NewUser } from './NewUser';
import { UserDetails } from './UserDetails';
import { UsersList } from './UsersList';
import { EditUser } from './EditUser';

const usersRouter = createBrowserRouter([
  {
    path: '/',
    element: <UsersLayout />,
    children: [
      { index: true, element: <UsersList /> },
      { path: 'new', element: <NewUser /> },
      {
        path: ':userId/*',
        loader: ({ params }) =>
          fetch(`/api/users/${params.userId}`).then((res) => res.json()),
        children: [
          { index: true, element: <UserDetails /> },
          {
            path: 'edit',
            element: <EditUser />,
          },
        ],
      },
    ],
  },
]);

export default usersRouter;

// main.tsx
// lazy-load usersRouter, users-related components are not included in the initial bundle
const usersRouter = () => import('./users/routes.tsx')

const rootRouter = createBrowserRouter([
  {
    path: '/',
    element: <Layout />,
    loader: () => fetch(`/api/config`).then((res) => res.json()),
    children: [
      { index: true, element: <Home /> },
      // `router` should be able to accept lazy-loaded routers
      // `router` and `element` are mutually exclusive
      { path: 'users/*', router: usersRouter }, 
    ],
  },
  {
    path: '/login'',
    element: <Login />,
  }
]);

@tlmader
Copy link

tlmader commented Oct 19, 2022

We have an application that uses lazy-loaded descendant routes. I am running into this scenario when attempting to take advantage of the new useMatches hook for breadcrumbs.

@brophdawg11
Copy link
Contributor

I'm going to convert this to a discussion so it can go through our new Open Development process. Please upvote the new Proposal if you'd like to see this considered!

@remix-run remix-run locked and limited conversation to collaborators Jan 9, 2023
@brophdawg11 brophdawg11 converted this issue into discussion #9855 Jan 9, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
Projects
None yet
Development

No branches or pull requests

5 participants