diff --git a/.changeset/cool-onions-compete.md b/.changeset/cool-onions-compete.md new file mode 100644 index 000000000..bb0ea0e77 --- /dev/null +++ b/.changeset/cool-onions-compete.md @@ -0,0 +1,6 @@ +--- +"@squide/react-router": patch +"@squide/firefly": patch +--- + +Improved the no custom 404 route handling. diff --git a/packages/firefly/src/AppRouter.tsx b/packages/firefly/src/AppRouter.tsx index 318401da3..ac975f733 100644 --- a/packages/firefly/src/AppRouter.tsx +++ b/packages/firefly/src/AppRouter.tsx @@ -1,6 +1,6 @@ import { isNil, useLogOnceLogger } from "@squide/core"; import { useIsMswStarted } from "@squide/msw"; -import { useIsRouteProtected, useRouteMatch, useRoutes, type Route } from "@squide/react-router"; +import { findRouteByPath, useIsRouteProtected, useRouteMatch, useRoutes, type Route } from "@squide/react-router"; import { useAreModulesReady, useAreModulesRegistered } from "@squide/webpack-module-federation"; import { cloneElement, useCallback, useEffect, useMemo, type ReactElement } from "react"; import { ErrorBoundary, useErrorBoundary } from "react-error-boundary"; @@ -225,14 +225,11 @@ export function AppRouter(props: AppRouterProps) { // HACK: // When there's a direct hit on a deferred route, since the route has not been registered yet (because it's a deferred registration), // the React Router router instance doesn't know about that route and will therefore fallback to the no match route. - // If there's no custom no match route defined with path="*", React Router will not even bother trying to render a route and will defer to - // it's default no match route, which breaks the AppRouter logic. - // To circumvent this issue, if the application doesn't register a no match route, Squide add one by default. - if (!routes.some(x => x.path === "*")) { - routes.push({ - path: "*", - lazy: () => import("./NoMatchRouteFallback.tsx") - }); + // If there's isn't a custom no match route defined with path="*", React Router will fallback it's default no match router instead + // of rendering a route which will break the AppRouter component. + // To circumvent this issue, if the application doesn't register a custom no match route, an Error is thrown. + if (areModulesRegistered && !findRouteByPath(routes, "*")) { + throw new Error("For the AppRouter component to work properly, the application must be a define a custom no match router. For additional information, refer to: https://reactrouter.com/en/main/start/tutorial#handling-not-found-errors."); } return renderRouterProvider([ diff --git a/packages/firefly/src/NoMatchRouteFallback.tsx b/packages/firefly/src/NoMatchRouteFallback.tsx deleted file mode 100644 index 624493ed1..000000000 --- a/packages/firefly/src/NoMatchRouteFallback.tsx +++ /dev/null @@ -1,18 +0,0 @@ -export function NoMatchRouteFallback() { - return ( -
-

404 not found

-

This page has been dynamically added by Squide to fix an issue with the AppRouter component. Please replace this page in your application by a custom match page.

-

The code should be like the following:

-
-{`runtime.registerRoute({
-    $visibility: "public",
-    path: "*",
-    lazy: import("./NoMatchPage.tsx")
-});`}
-            
-
- ) -} - -export const Component = NoMatchRouteFallback; diff --git a/packages/firefly/src/index.ts b/packages/firefly/src/index.ts index 8a046b335..ae0d9da33 100644 --- a/packages/firefly/src/index.ts +++ b/packages/firefly/src/index.ts @@ -4,7 +4,5 @@ export * from "@squide/react-router"; export * from "@squide/webpack-module-federation"; export * from "./AppRouter.tsx"; -export * from "./NoMatchRouteFallback.tsx"; - export * from "./fireflyRuntime.tsx"; diff --git a/packages/firefly/tests/AppRouter.test.tsx b/packages/firefly/tests/AppRouter.test.tsx index 56a39121f..9a0b9551f 100644 --- a/packages/firefly/tests/AppRouter.test.tsx +++ b/packages/firefly/tests/AppRouter.test.tsx @@ -56,10 +56,9 @@ beforeEach(() => { test("when no data handlers are provided, msw is disabled, there's no deferred registrations, and modules are not registered yet, render the fallback", async () => { const runtime = new FireflyRuntime(); - // Must add at least a route otherwise useRouteMatchProtected will throw. runtime.registerRoute({ path: "*", - element:
A wildcard route
+ element:
A custom no match route
}, { hoist: true }); @@ -77,10 +76,9 @@ test("when no data handlers are provided, msw is disabled, there's no deferred r test("when no data handlers are provided, msw is disabled, there's no deferred registrations, and modules are registered, render the router", async () => { const runtime = new FireflyRuntime(); - // Must add at least a route otherwise useRouteMatchProtected will throw. runtime.registerRoute({ path: "*", - element:
A wildcard route
+ element:
A custom no match route
}, { hoist: true }); @@ -104,10 +102,9 @@ test("when no data handlers are provided, msw is disabled, there's no deferred r test("when no data handlers are provided, msw is disabled, modules are registered but there's uncompleted deferred registrations, render the fallback", async () => { const runtime = new FireflyRuntime(); - // Must add at least a route otherwise useRouteMatchProtected will throw. runtime.registerRoute({ path: "*", - element:
A wildcard route
+ element:
A custom no match route
}, { hoist: true }); @@ -124,11 +121,9 @@ test("when no data handlers are provided, msw is disabled, modules are registere test("when a onLoadPublicData handler is provided and the public data is not loaded, render the fallback element", async () => { const runtime = new FireflyRuntime(); - // Must add at least a route otherwise useRouteMatchProtected will throw. runtime.registerRoute({ - $visibility: "public", path: "*", - element:
A wildcard route
+ element:
A custom no match route
}, { hoist: true }); @@ -148,6 +143,13 @@ test("when a onLoadPublicData handler is provided and the public data is not loa test("when a onLoadPublicData handler is provided and the public data is loaded, render the router", async () => { const runtime = new FireflyRuntime(); + runtime.registerRoute({ + path: "*", + element:
A custom no match route
+ }, { + hoist: true + }); + runtime.registerRoute({ $visibility: "public", index: true, @@ -178,10 +180,9 @@ test("when a onLoadPublicData handler is provided and the public data is loaded, test("when a onLoadProtectedData handler is provided and the protected data is not loaded, render the fallback element", async () => { const runtime = new FireflyRuntime(); - // Must add at least a route otherwise React Router complains the router has no routes. runtime.registerRoute({ path: "*", - element:
A wildcard route
+ element:
A custom no match route
}, { hoist: true }); @@ -201,6 +202,13 @@ test("when a onLoadProtectedData handler is provided and the protected data is n test("when a onLoadProtectedData handler is provided and the protected data is loaded, render the router", async () => { const runtime = new FireflyRuntime(); + runtime.registerRoute({ + path: "*", + element:
A custom no match route
+ }, { + hoist: true + }); + runtime.registerRoute({ index: true, element:
A route
@@ -230,10 +238,9 @@ test("when a onLoadProtectedData handler is provided and the protected data is l test("when msw is enabled and msw is not started, render the fallback element", async () => { const runtime = new FireflyRuntime(); - // Must add at least a route otherwise React Router complains the router has no routes. runtime.registerRoute({ path: "*", - element:
A wildcard route
+ element:
A custom no match route
}, { hoist: true }); @@ -250,6 +257,13 @@ test("when msw is enabled and msw is not started, render the fallback element", test("when msw is enabled and msw is started, render the router", async () => { const runtime = new FireflyRuntime(); + runtime.registerRoute({ + path: "*", + element:
A custom no match route
+ }, { + hoist: true + }); + runtime.registerRoute({ index: true, element:
A route
@@ -271,6 +285,13 @@ test("when msw is enabled and msw is started, render the router", async () => { test("when a onCompleteRegistrations handler is provided and there's no deferred registrations, render the router", async () => { const runtime = new FireflyRuntime(); + runtime.registerRoute({ + path: "*", + element:
A custom no match route
+ }, { + hoist: true + }); + runtime.registerRoute({ index: true, element:
A route
@@ -291,10 +312,9 @@ test("when a onCompleteRegistrations handler is provided and there's no deferred test("when a onCompleteRegistrations handler is provided and the deferred registrations are not completed, render the fallback element", async () => { const runtime = new FireflyRuntime(); - // Must add at least a route otherwise React Router complains the router has no routes. runtime.registerRoute({ path: "*", - element:
A wildcard route
+ element:
A custom no match route
}, { hoist: true }); @@ -313,10 +333,9 @@ test("when a onCompleteRegistrations handler is provided and the deferred regist test("when a onCompleteRegistrations handler is provided and the deferred registrations are completed, render the router", async () => { const runtime = new FireflyRuntime(); - // Must add at least a route otherwise React Router complains the router has no routes. runtime.registerRoute({ path: "*", - element:
A wildcard route
+ element:
A custom no match route
}, { hoist: true }); @@ -352,10 +371,9 @@ test("when a onCompleteRegistrations handler is provided and the deferred regist test("when a onCompleteRegistrations handler is provided and a onLoadPublicData handler is provided, do not complete the deferred registrations and render the route until the public date is loaded", async () => { const runtime = new FireflyRuntime(); - // Must add at least a route otherwise React Router complains the router has no routes. runtime.registerRoute({ path: "*", - element:
A wildcard route
+ element:
A custom no match route
}, { hoist: true }); @@ -411,10 +429,9 @@ test("when a onCompleteRegistrations handler is provided and a onLoadPublicData test("when a onCompleteRegistrations handler is provided and a onLoadProtectedData handler is provided, do not complete the deferred registrations and render the route until the protected date is loaded", async () => { const runtime = new FireflyRuntime(); - // Must add at least a route otherwise React Router complains the router has no routes. runtime.registerRoute({ path: "*", - element:
A wildcard route
+ element:
A custom no match route
}, { hoist: true }); @@ -470,10 +487,9 @@ test("when a onCompleteRegistrations handler is provided and a onLoadProtectedDa test("when a onCompleteRegistrations handler is provided, a onLoadPublicData handler and a onLoadProtectedData handler are provided, do not complete the deferred registrations and render the route until the public and protected date are loaded", async () => { const runtime = new FireflyRuntime(); - // Must add at least a route otherwise React Router complains the router has no routes. runtime.registerRoute({ path: "*", - element:
A wildcard route
+ element:
A custom no match route
}, { hoist: true }); @@ -540,11 +556,9 @@ test("when an error occurs while loading the public data, render the error eleme const runtime = new FireflyRuntime(); - // Must add at least a route otherwise useRouteMatchProtected will throw. runtime.registerRoute({ - $visibility: "public", path: "*", - element:
A wildcard route
+ element:
A custom no match route
}, { hoist: true }); @@ -569,10 +583,9 @@ test("when an error occurs while loading the protected data, render the error el const runtime = new FireflyRuntime(); - // Must add at least a route otherwise useRouteMatchProtected will throw. runtime.registerRoute({ path: "*", - element:
A wildcard route
+ element:
A custom no match route
}, { hoist: true }); @@ -589,3 +602,13 @@ test("when an error occurs while loading the protected data, render the error el spy.mockRestore(); }); + +test("throw an error if no custom no match route are registered", async () => { + const runtime = new FireflyRuntime(); + + await registerLocalModules([() => {}], runtime); + + expect(() => renderWithRuntime(runtime, createAppRouter({ + waitForMsw: false + }))).toThrow(/For the AppRouter component to work properly, the application must be a define a custom no match router/); +}); diff --git a/packages/react-router/src/findRouteByPath.ts b/packages/react-router/src/findRouteByPath.ts new file mode 100644 index 000000000..ef38d2d7c --- /dev/null +++ b/packages/react-router/src/findRouteByPath.ts @@ -0,0 +1,19 @@ +import { Route } from "./routeRegistry.ts"; + +export function findRouteByPath(routes: Route[], path: string): Route | undefined { + for (const route of routes.values()) { + if (route.path === path) { + return route; + } + + if (route.children) { + const childRoute = findRouteByPath(route?.children, path); + + if (childRoute) { + return childRoute; + } + } + } + + return undefined; +} diff --git a/packages/react-router/src/index.ts b/packages/react-router/src/index.ts index 3a12c5957..ccc90d884 100644 --- a/packages/react-router/src/index.ts +++ b/packages/react-router/src/index.ts @@ -1,3 +1,4 @@ +export * from "./findRouteByPath.ts"; export * from "./navigationItemRegistry.ts"; export * from "./outlets.ts"; export * from "./reactRouterRuntime.ts"; diff --git a/packages/react-router/src/useIsRouteProtected.ts b/packages/react-router/src/useIsRouteProtected.ts index 0437454e8..be86a7630 100644 --- a/packages/react-router/src/useIsRouteProtected.ts +++ b/packages/react-router/src/useIsRouteProtected.ts @@ -3,20 +3,20 @@ import { Route } from "./routeRegistry.ts"; export function useIsRouteProtected(route?: Route) { if (!route) { // HACK: - // An unregistrered route will be considered as "protected" by default to facilitate the implementation of deferred routes. + // An unregistrered route is considered as "protected" by default to facilitate the implementation of deferred routes. // The issue is that when there's a direct hit on a deferred route, it cannot be determined whether or not a deferred route is public or protected // because the deferred route hasn't been registered yet (since it's a deferred route). - // If that deferred route depends on protected data, if we don't return "true" here, the deferred route will be registered without providing the protected data - // which will cause a runtime error. + // If that deferred route depends on protected data, if we don't return "true" here, the deferred route will be registered before the protected data + // is loaded which will probably cause a runtime error. return true; } if (route.path === "*") { // HACK: - // With the current AppRouter implementation, when there's a direct hit to a deferred route, since the route has not been registered yet to - // the React Router router instance, the router is trying to render the no match route which confuse this hook as it would return a boolean value - // for the visibility of the no match route (which is usually public) rather than the visiblity of the route asked by the consumer (which may be - // protected). To circumvent this issue, true is returned for no match route. Anyway, that would really make sense to direct hit the no match route. + // With the current AppRouter component implementation, when there's a direct hit to a deferred route, since the route has not been registered yet to + // the React Router router instance, the router is trying to render the no match route. Therefore this hook returns the a boolean value based + // on the visibility status of the no match route instead of the actual page request by the user. + // To circumvent this issue, "true" is returned for the no match route. return true; }