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

[Bug]: Route id collision when using React.Fragment with data router #10111

Closed
afn opened this issue Feb 17, 2023 · 4 comments · Fixed by #10193
Closed

[Bug]: Route id collision when using React.Fragment with data router #10111

afn opened this issue Feb 17, 2023 · 4 comments · Fixed by #10193
Labels

Comments

@afn
Copy link

afn commented Feb 17, 2023

What version of React Router are you using?

6.8.1

Steps to Reproduce

  1. Use createRoutesFromElements() with React.Fragments — for example, modify the dev/examples/data-router example to wrap one of the routes inside a React.Fragment:
let router = createBrowserRouter(
  createRoutesFromElements(
    <Route path="/" element={<Layout />}>
      <Route index loader={homeLoader} element={<Home />} />
      <React.Fragment>
        <Route
          path="todos"
          action={todosAction}
          loader={todosLoader}
          element={<TodosList />}
          errorElement={<TodosBoundary />}
        >
          <Route path=":id" loader={todoLoader} element={<Todo />} />
        </Route>
      </React.Fragment>
      <Route
        path="deferred"
        loader={deferredLoader}
        element={<DeferredPage />}
      />
    </Route>
  )
);

Expected Behavior

No errors are raised, and the router behaves the same as if the routes within the fragment were not nested within a fragment.

Actual Behavior

Raises the error:

Uncaught Error: Found a route id collision on id "0-0".  Route id's must be globally unique within Data Router usages
    at invariant (react-router-dom.js?v=17e401c6:197:11)
    at react-router-dom.js?v=17e401c6:409:5
    at Array.map (<anonymous>)
    at convertRoutesToDataRoutes (react-router-dom.js?v=17e401c6:405:17)
    at react-router-dom.js?v=17e401c6:419:36
    at Array.map (<anonymous>)
    at convertRoutesToDataRoutes (react-router-dom.js?v=17e401c6:405:17)
    at createRouter (react-router-dom.js?v=17e401c6:1009:20)
    at createBrowserRouter (react-router-dom.js?v=17e401c6:3634:10)
    at app.tsx:30:14

I believe the issue stems from the fact that the ids are generated based on the position of the routes within the DOM, but in the case of fragments, the React.Fragment element is omitted in the path, so (in the above example) both the Home and TodosList routes have the path 0-0.

@afn afn added the bug label Feb 17, 2023
@afn
Copy link
Author

afn commented Feb 17, 2023

I think the fix is simply:

diff --git a/packages/react-router/lib/components.tsx b/packages/react-router/lib/components.tsx
index b76e5781..e41405f2 100644
--- a/packages/react-router/lib/components.tsx
+++ b/packages/react-router/lib/components.tsx
@@ -563,7 +563,7 @@ export function createRoutesFromChildren(
       // Transparently support React.Fragment and its children.
       routes.push.apply(
         routes,
-        createRoutesFromChildren(element.props.children, parentPath)
+        createRoutesFromChildren(element.props.children, [...parentPath, index])
       );
       return;
     }

@AndrewCraswell
Copy link

I also just encountered this bug. Same exact situation it seems:

export const routes = createHashRouter(
  createRoutesFromElements(
    <Route path="/" element={<AppShell />}>
      {/* These routes are generally available */}
      <Route path="people" element={<People />}>
        <Route path="users/*" element={Outlet} />
        <Route path="memberships/*" element={Outlet} />
      </Route>

      {/* These routes only available if "access is not enabled" has not caught */}
      {hasAccess() ? (
        <>
          <Route index element={<Homepage />} />

          <Route path="runs" element={<RunHealth />} />

          <Route path="pipelines" element={<RunPipelines />} />
        </>
      ) : (
        <Route path="*" element={<ReturnToProductMessage />} />
      )}

      {/* If route is not handled, display 404 return to home error */}
      <Route path="*" element={<ReturnToHomeMessage />} />
    </Route>
  )
);

@AndrewCraswell
Copy link

AndrewCraswell commented Mar 28, 2023

I also just encountered this bug. Same exact situation it seems:

export const routes = createHashRouter(
  createRoutesFromElements(
    <Route path="/" element={<AppShell />}>
      {/* These routes are generally available */}
      <Route path="people" element={<List />} />

      {has Access() ? (
        <>
          <Route index element={<Home />} />

          <Route path="runs" element={<Profile />} />

          <Route path="pipelines" element={<Settings/>} />
        </>
      ) : (
        <Route path="*" element={<NoAccess />} />
      )}

      <Route path="*" element={<Error404 />} />
    </Route>
  )
);

Seems like maybe there is a fix and it's just awaiting release? Is there a scheduled time?

EDIT: Oh I see it made it into the current pre-release. Tested and verified that the fix seems to resolve the issue. Thanks!

@brophdawg11
Copy link
Contributor

This should be fixed in 6.10.0 👍

@brophdawg11 brophdawg11 removed the awaiting release This issue have been fixed and will be released soon label Mar 29, 2023
@brophdawg11 brophdawg11 removed their assignment Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants