[Bug]: unstable_patchRoutesOnMiss remote module error-handling

[VillePakarinenPosti]

What version of React Router are you using?

6.24.1

Steps to Reproduce

Guide on createBrowserRoute docs using Co-locating route discovery with route definition

      unstable_patchRoutesOnMiss: async ({ matches, path, patch }) => {
        const leafRoute = matches[matches.length - 1]?.route
        const maybeLazyChildren = leafRoute?.handle?.lazyChildren
        if (maybeLazyChildren) {
          const dynamicRoutes = await maybeLazyChildren()
          return patch(leafRoute.id, dynamicRoutes)
        }
      }

Expected Behavior

When maybeLazyChildren throws an error eg. due to network error within unstable_patchRoutesOnMiss, it should allow the error to be captured by one of the error-boundaries defined for the router. This would allows users to customize the error-handling behaviour similarly as with current lazy based approach.

Actual Behavior

Throwing an error within unstable_patchRoutesOnMiss results in the same behaviour as returning undefined from the function.

One option I was experimenting with was to handle the error in the userland within the unstable_patchRoutesOnMiss and return a element showing an error-screen instead, but this wouldn't allow users to retry & recover since the discovery happens just once.

[brophdawg11]

Could you provide a reproduction? Errors thrown from unstable_patchRoutesOnMiss should bubble to error boundaries just like errors thrown from loaders/actions. This is working correctly in my simple reproduction: https://codesandbox.io/p/sandbox/sleepy-solomon-94qqsw?file=%2Fsrc%2Findex.js%3A26%2C24

[VillePakarinenPosti]

Link to reproduction

[brophdawg11]

ah ok it looks like a bug related to the bubbling of the error - I'll get a fix in for this shortly.

also, just FYI you don't need a splat on your path to use this - you could just use path: "dynamic" if you wanted to

[VillePakarinenPosti]

ah ok it looks like a bug related to the bubbling of the error - I'll get a fix in for this shortly.

That is great!

also, just FYI you don't need a splat on your path to use this - you could just use path: "dynamic" if you wanted to

Adding the splat(*) was the only way for me to get lazily discovered index routes to work. Is this intended behaviour? See more from: https://codesandbox.io/p/sandbox/silly-gauss-4wjjvk

[brophdawg11]

ah - yeah you could solve it that way but I don't think I'd recommend it. That makes it tricky if you ever intended to have a splat in the tree you patch in beneath that.

The issue you're running into is that patchRoutesOnMiss only runs when it doesn't match. If you include a parent route in the tree but not the index route - then it does match so it never needs to call patchRoutesOnMiss because it didn't "miss"

So I would either include both the parent and it's index route in the critical bundle, or neither. but don't just include the parent in the critical bundle:

// Include both - this is my suggestion for a root route (`/`) since that's usually 
// the route folks will land on as a starting point so it's worth avoiding a 
// network waterfall to fetch the homepage IMO
let routes = createBrowserRouter([{
  path: '/',
  Component: Root,
  children: [{
    index: true,   
    Component: Index
  }]
}], {
  unstable_patchRoutesOnMiss({ patch }) {
    // You can then patch addition children into the root route here as needed
    // patch('root', someRoutes);
  }
});

// Include neither - this is my suggestion for non root routes (`/parent`) since if there's no point 
// in including the parent without the index and if it's not a common initial landing page 
// then you don't want either in your critical bundle
let routes = createBrowserRouter([{
  path: '/',
  Component: Root,
  children: [{
    index: true,   
    Component: Index
  }]
}], {
  unstable_patchRoutesOnMiss({ path, patch }) {
    if (path === "/parent") {
      // Patch both the parent and it's index route in here
      // Also assume these routes are async loaded and thus not in the bundle :)
      patch(null, [{
        path: '/parent',
        Component: Parent,
        children: [{
          index: true,
          Component: ParentIndex
        }]
      });
    }
  }
});

[VillePakarinenPosti]

Thanks for the explanation.

It felt intuitive to have the parent in the critical bundle with splat; when patching a nested route like :lang/:org/dynamic. For a route config with some level of nesting, the handle based colocation allowed the patchRoutesOnMiss implementation to remain pretty slim and you could keep all the related logic within the config.

Anyway cudos for designing an flexible API which allows different types of tradeoffs!

[brophdawg11]

Yeah - to be honest that type of "use a splat to discover index" routes approach hadn't occurred to me during implementation and it should work. So if you don't intend to use splat routes below that insertion point it might be a fine solution.

I do like the handle.lazyChildren colocation, and the main concern with that is bloating the critical path by including the layout component in the bundle, so maybe an idea there is to just use a pathless layout as the sole child of those?

// app.jsx
const router = createBrowserRouter([{
  path: '/',
  Component: Root,
}, {
  path: '/dashboard',
  // No Component so we don't bloat the bundle and it'll just be an Outlet
  handle: {
    lazyChildren: () => import('./dashboard'),
  }
}], {
  unstable_patchRoutesOnMiss: async ({ matches, path, patch }) => {
   ...
  }
});

// dashboard.tsx
export default routes = [{
  path: '',
  // Dashboard Component. is moved down to a pathless layout route 
  // as a sole child of `/dashboard` so it's still in the lazy bundle
  Component: Dashboard,
  children: [{
    index: true,
    Component: DashboardIndex,
  }, {
    path: 'widgets',
    component : DashboardWidgets,
  }]
}];

That way the only thing that ends up in the critical bundle is the root level path values and the import() calls...

I think over time we'll see some common best practices emerge in this are area as folks start using this API.

[brophdawg11]

Fixed by #11786 and will be available in the next release

[github-actions]

🤖 Hello there,

We just published version 6.25.0-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[VillePakarinenPosti]

Thanks @brophdawg11. I tested that it now bubbles the error correctly to the expected error-boundary in 6.25.0-pre.0.

One improvement idea for the future would be to provide the discoverResult.error as Error.cause instead of stringifying the message within getInternalRouterError. This would help out when building fine grained error-views with useRouteError.

Thanks for the fix!

[brophdawg11]

Yeah that could be a good idea to keep in mind - maybe instead of being exposed as an 400 ErrorResponse it should just be exposed as an Error. Right now our TS setup doesn't recognize cause in the Error constructor so that would have to come along with an update to our TS config settings as well.

[VillePakarinenPosti]

Yeah that could be a good idea to keep in mind - maybe instead of being exposed as an 400 ErrorResponse it should just be exposed as an Error.

This would definetly be my preferred way on dealing with it, unless there are some other benefits of using ErrorResponse that I'm not aware of✌️

[github-actions]

🤖 Hello there,

We just published version 6.25.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!