Skip to content

Add support for Fragments and support route grouping#122

Closed
Fionoble wants to merge 3 commits intopreactjs:mainfrom
Fionoble:fio-fix-router-parsing
Closed

Add support for Fragments and support route grouping#122
Fionoble wants to merge 3 commits intopreactjs:mainfrom
Fionoble:fio-fix-router-parsing

Conversation

@Fionoble
Copy link

This pull request improves the router's ability to handle routes nested inside Fragment components or arbitrary wrapper elements, ensuring that all routes are recognized regardless of their nesting structure. It also adds comprehensive tests to verify this new behavior.

Router nesting support:

  • Updated src/router.js to recursively flatten and collect all Route components from nested children, including those inside Fragment or wrapper components, before matching routes. This ensures nested routes are now supported. [1] [2]

Testing improvements:

  • Added new tests in test/router.test.js to verify that routes nested within Fragment components, wrapper elements (like div), and deeply nested structures are correctly recognized and rendered by the router.
  • Added a separate test to ensure routes with mixed nesting and fragments work as expected, including default routes and various nesting combinations.

This now supports the following structure:

export function App() {
  return (
    <LocationProvider>
      <AuthProvider>
        <Header />
        <main>
          <Router>
            {/* Public Routes */}
            <Route path="/" component={Home} />
            <Route path="/about" component={About} />
            <Route path="/login" component={Login} />
						
            {/* Protected Routes - All routes requiring authentication */}
            <ProtectedRoutes>
              <Route path="/products" component={Products} />
              <Route path="/blog" component={Blog} />
              <Route path="/contact" component={Contact} />
            </ProtectedRoutes>
						
            <Route default component={NotFound} />
          </Router>
        </main>
      </AuthProvider>
    </LocationProvider>
  );
}

Copy link
Member

@rschristian rschristian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, but I'd be quite against adding anything like this.

<Router> should more or less act like a switch statement, matching direct descendants and choosing which of them is active. Adding arbitrary nesting a) reduces perf considerably for matching, as each sub tree now needs to be evaluated and b) deviates from that simplicity considerably.

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do generally like this kind of functionality as it would allow us to create similar patterns to react-router. Currently we're looking at ProtectedRoutes or ProtectedRoute, however I can see a case for doing <Route><Route /></Route> in future versions as DX wise it's quite neat to form these common layouts etc.

Not a right now thing. Generally the thing I like about allowing for this approach is that we can share common functionality among routes, be that an auth requirement or a common data-slice over context.

I do see Ryans point of simplicity here as we've tried keeping iso as small and performant as possible, though I don't think the performance should be highly impacted as this render method shouldn't invoke a whole lot. The size will def be impacted

@rschristian
Copy link
Member

Generally the thing I like about allowing for this approach is that we can share common functionality among routes, be that an auth requirement or a common data-slice over context.

Can do this pretty easily already by just creating an <AuthRoute> comp, no? All we do is look at the path and default props of Router's children, there's nothing that restricts you from creating your own wrappers. Can use a bunch of <div>s as your routes if you wanted.

That, and nested routers (where appropriate), have been what folks have been using to land that behavior without any issue to my knowledge; it's what I've been using to land that behavior without any real issue.

I don't think the performance should be highly impacted as this render method shouldn't invoke a whole lot

If you're using this w/ SSR (no idea if many people are, but nothing's stopping them) then that becomes a bigger concern. Unlikely to immediately be a bottleneck of course but trying to recursively search for children is a really weird default from a perf prospective as it'd be a fairly niche feature that's often searching dead-ends.


As for aligning with react-router, we already deviate in a million ways, I don't see any particular value in trying to align here in this specific area. AFAIK, RR generally just works with Preact and users can use it if they prefer the API.

This would be the sort of thing I'd want to veto, I really don't think it's a positive addition.

Fionoble and others added 2 commits September 11, 2025 12:05
Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>
Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>
@Fionoble
Copy link
Author

All good, just figured it would be a nice DX improvement to the library. There's no helpful error or feedback when you do this to other router conventions so adding support for it was my approach. I can close this if it's not desirable though.

@Fionoble Fionoble closed this Sep 11, 2025
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

Successfully merging this pull request may close these issues.

3 participants