[v6] [Feature] Explicitly warn/error against custom children in <Routes>

[JamesGelok]

It's clear from #8111 (comment) and #8092 (comment) that support for custom routes (or rendering anything other than a <Route/> or React.Fragment component as a child of <Routes>) is discouraged when someone asks about it.

Problem

I can see some people getting confused as to why this kind of thing wouldn't like most react components do.

toyExample.js

// 1. A Custom Route function
import { CustomRoute } from "./AnyOldCustomRouteFunction";

/** 2. Error Function that only throws */
const ErrFn = () => {
  throw new Error("I would throw an error but I can't because I'm never called.");
};

/** 3. Routes Component */
const MyRoutes = () => {
  return (
    <div style={{ fontFamily: "sans-serif" }}>
      Routes statically analyzes children but doesn't render them explicitly.
      <Routes>
        <Route path="/login" element={<h1>Login</h1>} />
        {/* ❗ Function is never called because it is statically analyzed. ❗ */}
        <ErrFn path="/valid" element={<h1>valid</h1>} />
        {/* ❗ Neither is this one because it is statically analyzed. ❗ */}        
        <CustomRoute path="/also-valid" element={<h1>also-valid</h1>} />
      </Routes>
    </div>
  );
}

Solution

In the example above, <Routes> receives an array of objects on the children prop

// > pseudo-code for children array
[
  {
    type: ƒ Route() {},
    key: null,
    ref: null,
    props: { path: "/login", element: Object },
    _owner: null,
    _store: Object,
  },
  {
    type: ƒ ErrFn() {},
    key: null,
    ref: null,
    props: { path: "/valid", element: Object },
    _owner: null,
    _store: Object,
  },
  {
    type: ƒ CustomRoute() {},
    key: null,
    ref: null,
    props: { path: "/also-valid", element: Object },
    _owner: null,
    _store: Object,
  },
];

In the createRoutesFromChildren function, we can modify it to also check for if element.type !== Route and warn in development mode.

Here's a rough version of what could be done:

    if (element.type === React.Fragment) {
      // Transparently support React.Fragment and its children.
      routes.push.apply(
        routes,
        createRoutesFromChildren(element.props.children)
      );
      return;
    } 
    // 💡 Add an invariant check after checking for a react fragment
    invariant(
      typeof element.type === "string" ? false : element.type.name !== Route.name,
      `[${
        typeof element?.type === "string" ? element?.type : element?.type.name
      }] is not a <Route> component. All component children of <Routes> must be a <Route> or <React.Fragment>`
    );

Additional Notes

Potentially just a warning could be issued.

We could also use warning instead of invariant but I noticed that the <Route> function itself uses invariant if it ever actually gets ran as a function or component (rather than used in any context where it would be statically analyzed).

The above solution doesn't immediately work locally.

Tried inserting the above solution and running tests locally, but ended up with a lot of red. Not sure exactly what would need to be done to implement this.

Potentially a Symbol or string constant could be attached as a key on the Routes function, and that could be checked? Not sure if that's ideal or desired but just ideas.

[mjackson]

I'd be open to adding an invariant to the code for this. You're welcome to submit a PR.

FWIW I wrote up a doc on route composition for anyone who may be interested: https://gist.github.com/mjackson/d54b40a094277b7afdd6b81f51a0393f

It explains our rationale for making this decision as well as an alternative pattern for doing whatever you were doing before with your custom Route-like element.

[mjackson]

It should probably just be

invariant(
  element.type === Route,
  '...'
);

[JamesGelok]

You're welcome to submit a PR.

Thanks, @mjackson! Opened a PR here #8238

---

FWIW I wrote up a doc on route composition for anyone who may be interested: gist.github.com/mjackson/d54b40a094277b7afdd6b81f51a0393f

Imo, it actually might be really helpful to include a link to that write up in the error message.

I didn't include it in the PR. However, I still think a link to the rational might help guide people into better patterns.

---

It should probably just be

invariant(
  element.type === Route,
  '...'
);

You're right 😅 all tests pass locally. I misread how invariant worked; I mistakenly started with element.type !== Route and went even more off track from there lol

[mjackson]

This was released in 6.0.1