diff --git a/.changeset/slimy-pumas-breathe.md b/.changeset/slimy-pumas-breathe.md new file mode 100644 index 000000000..dcc8d4f7b --- /dev/null +++ b/.changeset/slimy-pumas-breathe.md @@ -0,0 +1,5 @@ +--- +"@squide/firefly": patch +--- + +Fixing a remaining issue with deferred registrations that depends on protected data. diff --git a/.changeset/thin-shoes-call.md b/.changeset/thin-shoes-call.md new file mode 100644 index 000000000..179e27295 --- /dev/null +++ b/.changeset/thin-shoes-call.md @@ -0,0 +1,5 @@ +--- +"@squide/react-router": patch +--- + +Internal changes to `useRouteMatch` and `useIsRouteProtected`. diff --git a/docs/reference/default.md b/docs/reference/default.md index 6be01e395..1d0bace83 100644 --- a/docs/reference/default.md +++ b/docs/reference/default.md @@ -41,7 +41,7 @@ toc: - [ManagedRoutes](./routing/ManagedRoutes.md) - [useRenderedNavigationItems](./routing/useRenderedNavigationItems.md) - [useRouteMatch](./routing/useRouteMatch.md) -- [useIsRouteMatchProtected](./routing/useIsRouteMatchProtected.md) +- [useIsRouteProtected](./routing/useIsRouteProtected.md) ### Logging diff --git a/docs/reference/routing/useIsRouteMatchProtected.md b/docs/reference/routing/useIsRouteMatchProtected.md deleted file mode 100644 index d5d0c6560..000000000 --- a/docs/reference/routing/useIsRouteMatchProtected.md +++ /dev/null @@ -1,51 +0,0 @@ ---- -toc: - depth: 2-3 ---- - -# useIsRouteMatchProtected - -Execute [React Router's matching algorithm](https://reactrouter.com/en/main/utils/match-routes) against the registered routes and a given `location` to determine if any route match the location and whether or not that matching route is `protected`. - -> To take advantage of this hook, make sure to add a [$visibility hint](../runtime/runtime-class.md#register-a-public-route) to your public pages. - -## Reference - -```ts -const isProtected = useIsRouteMatchProtected(locationArg) -``` - -### Parameters - -- `locationArg`: The location to match the route paths against. -- `options`: An optional object literal of options: - - `throwWhenThereIsNoMatch`: Whether or not to throw an `Error` if no route match `locationArg`. - -### Returns - -A `boolean` value indicating whether or not the matching route is `protected`. If `throwWhenThereIsNoMatch` is enabled and no route match the given location, an `Error` is thrown. - -If `throwWhenThereIsNoMatch` is disabled and there's no route matching `locationArg`, `true` is returned. - -## Usage - -### Using `useLocation` - -```ts -import { useLocation } from "react-router-dom"; -import { useIsRouteMatchProtected } from "@squide/firefly"; - -const location = useLocation(); - -// Returns true if the matching route doesn't have a $visibility: "public" property. -const isActiveRouteProtected = useIsRouteMatchProtected(location); -``` - -### Using `window.location` - -```ts -import { useIsRouteMatchProtected } from "@squide/firefly"; - -// Returns true if the matching route doesn't have a $visibility: "public" property. -const isActiveRouteProtected = useIsRouteMatchProtected(window.location); -``` diff --git a/docs/reference/routing/useIsRouteProtected.md b/docs/reference/routing/useIsRouteProtected.md new file mode 100644 index 000000000..fa6f55b86 --- /dev/null +++ b/docs/reference/routing/useIsRouteProtected.md @@ -0,0 +1,37 @@ +--- +order: -100 +toc: + depth: 2-3 +--- + +# useIsRouteProtected + +Determine whether or not a route is considered as `protected`. + +> To take advantage of this hook, make sure to add a [$visibility hint](../runtime/runtime-class.md#register-a-public-route) to your public pages. + +## Reference + +```ts +const isProtected = useIsRouteProtected(route) +``` + +### Parameters + +- `route`: A `Route` object. + +### Returns + +A `boolean` value indicating whether or not the matching route is `protected`. + +## Usage + +```ts +import { useLocation } from "react-router-dom"; +import { useIsRouteProtected, useRouteMatch } from "@squide/firefly"; + +const location = useLocation(); +const route = useRouteMatch(location); + +const isActiveRouteProtected = useIsRouteProtected(route); +``` diff --git a/docs/reference/routing/useRouteMatch.md b/docs/reference/routing/useRouteMatch.md index 62d8d7cc1..7123c2457 100644 --- a/docs/reference/routing/useRouteMatch.md +++ b/docs/reference/routing/useRouteMatch.md @@ -16,10 +16,14 @@ const match = useRouteMatch(locationArg) ### Parameters - `locationArg`: The location to match the route paths against. +- `options`: An optional object literal of options: + - `throwWhenThereIsNoMatch`: Whether or not to throw an `Error` if no route match `locationArg`. ### Returns -A `Route` object if there's a matching route, otherwise an `Error` is thrown. +A `Route` object if there's a matching route, otherwise if `throwWhenThereIsNoMatch` is enabled and no route match the given location, an `Error` is thrown. + +If `throwWhenThereIsNoMatch` is disabled and there's no route matching `locationArg`, `undefined` is returned. ## Usage @@ -30,6 +34,7 @@ import { useLocation } from "react-router-dom"; import { useRouteMatch } from "@squide/firefly"; const location = useLocation(); + const activeRoute = useRouteMatch(location); ``` diff --git a/packages/firefly/src/AppRouter.tsx b/packages/firefly/src/AppRouter.tsx index 0f6ddb574..318401da3 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 { useIsRouteMatchProtected, useRoutes, type Route } from "@squide/react-router"; +import { 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"; @@ -154,7 +154,8 @@ export function BootstrappingRoute(props: BootstrappingRouteProps) { useLoadPublicData(areModulesRegistered, areModulesReady, isMswStarted, isPublicDataLoaded, onLoadPublicData); // Only throw when there's no match if the modules has been registered, otherwise it's expected that there are no registered routes. - const isActiveRouteProtected = useIsRouteMatchProtected(location, { throwWhenThereIsNoMatch: areModulesReady }); + const activeRoute = useRouteMatch(location, { throwWhenThereIsNoMatch: areModulesReady }); + const isActiveRouteProtected = useIsRouteProtected(activeRoute); // Try to load the protected data if an handler is defined. useLoadProtectedData(areModulesRegistered, areModulesReady, isMswStarted, isActiveRouteProtected, isProtectedDataLoaded, onLoadProtectedData); @@ -170,7 +171,7 @@ export function BootstrappingRoute(props: BootstrappingRouteProps) { } }, [areModulesRegistered, areModulesReady, isMswStarted, isPublicDataLoaded, isProtectedDataLoaded, isActiveRouteProtected, onCompleteRegistrations]); - if (!areModulesReady || !isMswStarted || !isPublicDataLoaded || (isActiveRouteProtected && !isProtectedDataLoaded)) { + if (!areModulesReady || !isMswStarted || !activeRoute || !isPublicDataLoaded || (isActiveRouteProtected && !isProtectedDataLoaded)) { return fallbackElement; } @@ -221,6 +222,19 @@ export function AppRouter(props: AppRouterProps) { }, [errorElement]); return useMemo(() => { + // 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") + }); + } + return renderRouterProvider([ { element: ( diff --git a/packages/firefly/src/NoMatchRouteFallback.tsx b/packages/firefly/src/NoMatchRouteFallback.tsx new file mode 100644 index 000000000..624493ed1 --- /dev/null +++ b/packages/firefly/src/NoMatchRouteFallback.tsx @@ -0,0 +1,18 @@ +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 ae0d9da33..8a046b335 100644 --- a/packages/firefly/src/index.ts +++ b/packages/firefly/src/index.ts @@ -4,5 +4,7 @@ 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/react-router/src/index.ts b/packages/react-router/src/index.ts index 963aaa2e3..3a12c5957 100644 --- a/packages/react-router/src/index.ts +++ b/packages/react-router/src/index.ts @@ -2,7 +2,7 @@ export * from "./navigationItemRegistry.ts"; export * from "./outlets.ts"; export * from "./reactRouterRuntime.ts"; export * from "./routeRegistry.ts"; -export * from "./useIsRouteMatchProtected.ts"; +export * from "./useIsRouteProtected.ts"; export * from "./useNavigationItems.ts"; export * from "./useRenderedNavigationItems.tsx"; export * from "./useRouteMatch.ts"; diff --git a/packages/react-router/src/useIsRouteMatchProtected.ts b/packages/react-router/src/useIsRouteMatchProtected.ts deleted file mode 100644 index 813be08b9..000000000 --- a/packages/react-router/src/useIsRouteMatchProtected.ts +++ /dev/null @@ -1,24 +0,0 @@ -import { useRouteMatch } from "./useRouteMatch.ts"; - -export interface UseIsRouteMatchProtectedOptions { - throwWhenThereIsNoMatch?: boolean; -} - -export function useIsRouteMatchProtected(locationArg: Partial, { throwWhenThereIsNoMatch = true } = {}) { - const activeRoute = useRouteMatch(locationArg); - - if (!activeRoute) { - if (throwWhenThereIsNoMatch) { - throw new Error(`[squide] There's no matching route for the location: "${locationArg.pathname}". Did you add routes to React Router without using the runtime.registerRoute() function?`); - } - - // An unregistrered route will be 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. - return true; - } - - return activeRoute.$visibility === "protected"; -} diff --git a/packages/react-router/src/useIsRouteProtected.ts b/packages/react-router/src/useIsRouteProtected.ts new file mode 100644 index 000000000..0437454e8 --- /dev/null +++ b/packages/react-router/src/useIsRouteProtected.ts @@ -0,0 +1,24 @@ +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. + // 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. + 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. + return true; + } + + return route.$visibility === "protected"; +} diff --git a/packages/react-router/src/useRouteMatch.ts b/packages/react-router/src/useRouteMatch.ts index 70ed98e56..111f58bf5 100644 --- a/packages/react-router/src/useRouteMatch.ts +++ b/packages/react-router/src/useRouteMatch.ts @@ -1,7 +1,11 @@ import { matchRoutes } from "react-router-dom"; import { useRoutes } from "./useRoutes.ts"; -export function useRouteMatch(locationArg: Partial) { +export interface UseIsRouteMatchOptions { + throwWhenThereIsNoMatch?: boolean; +} + +export function useRouteMatch(locationArg: Partial, { throwWhenThereIsNoMatch = true }: UseIsRouteMatchOptions = {}) { const routes = useRoutes(); const matchingRoutes = matchRoutes(routes, locationArg) ?? []; @@ -10,6 +14,10 @@ export function useRouteMatch(locationArg: Partial) { // When a route is nested, it also returns all the parts that constituate the whole route (for example the layouts and the boundaries). // We only want to know the visiblity of the actual route that has been requested, which is always the last entry. return matchingRoutes[matchingRoutes.length - 1]!.route; + } else { + if (throwWhenThereIsNoMatch) { + throw new Error(`[squide] There's no matching route for the location: "${locationArg.pathname}". Did you add routes to React Router without using the runtime.registerRoute() function?`); + } } return undefined; diff --git a/samples/endpoints/local-module/src/register.tsx b/samples/endpoints/local-module/src/register.tsx index a5c426ae1..c333fb349 100644 --- a/samples/endpoints/local-module/src/register.tsx +++ b/samples/endpoints/local-module/src/register.tsx @@ -71,6 +71,10 @@ function registerRoutes(runtime: FireflyRuntime, i18nextInstance: i18n): Deferre }); return ({ featureFlags } = {}) => { + if (!runtime.getSession()) { + throw new Error("The deferred registratons are broken again as they are executed before the protected data has been loaded."); + } + if (featureFlags?.featureA) { runtime.registerRoute({ path: "/feature-a", diff --git a/samples/endpoints/remote-module/src/register.tsx b/samples/endpoints/remote-module/src/register.tsx index cc5045502..eafb7c0b4 100644 --- a/samples/endpoints/remote-module/src/register.tsx +++ b/samples/endpoints/remote-module/src/register.tsx @@ -109,6 +109,10 @@ function registerRoutes(runtime: FireflyRuntime, i18nextInstance: i18n): Deferre }); return ({ featureFlags } = {}) => { + if (!runtime.getSession()) { + throw new Error("The deferred registratons are broken again as they are executed before the protected data has been loaded."); + } + if (featureFlags?.featureB) { runtime.registerRoute({ path: "/feature-b", diff --git a/samples/endpoints/shell/src/AppRouter.tsx b/samples/endpoints/shell/src/AppRouter.tsx index 1161578af..8b7568567 100644 --- a/samples/endpoints/shell/src/AppRouter.tsx +++ b/samples/endpoints/shell/src/AppRouter.tsx @@ -6,6 +6,7 @@ import { useTranslation } from "react-i18next"; import { RouterProvider, createBrowserRouter } from "react-router-dom"; import { AppRouterErrorBoundary } from "./AppRouterErrorBoundary.tsx"; import { i18NextInstanceKey } from "./i18next.ts"; +import { useRefState } from "./useRefState.tsx"; export interface DeferredRegistrationData { featureFlags?: FeatureFlags; @@ -55,6 +56,7 @@ async function fetchSubscription(signal: AbortSignal) { function fetchProtectedData( setSession: (session: Session) => void, setSubscription: (subscription: Subscription) => void, + setIsProtectedDataLoaded: (isProtectedDataLoaded: boolean) => void, signal: AbortSignal, logger: Logger ) { @@ -70,6 +72,7 @@ function fetchProtectedData( logger.debug("[shell] %cSubscription is ready%c:", "color: white; background-color: green;", "", subscription); setSubscription(subscription); + setIsProtectedDataLoaded(true); }) .catch((error: unknown) => { if (isApiError(error) && error.status === 401) { @@ -92,7 +95,9 @@ function Loading() { export function AppRouter({ waitForMsw, sessionManager, telemetryService }: AppRouterProps) { const [featureFlags, setFeatureFlags] = useState(); - const [subscription, setSubscription] = useState(); + + const [subscriptionRef, setSubscription] = useRefState(); + const [isProtectedDataLoaded, setIsProtectedDataLoaded] = useState(false); const logger = useLogger(); const runtime = useRuntime(); @@ -112,7 +117,7 @@ export function AppRouter({ waitForMsw, sessionManager, telemetryService }: AppR changeLanguage(session.user.preferredLanguage); }; - return fetchProtectedData(setSession, setSubscription, signal, logger); + return fetchProtectedData(setSession, setSubscription, setIsProtectedDataLoaded, signal, logger); }, [logger, sessionManager, changeLanguage]); const handleCompleteRegistrations = useCallback(() => { @@ -124,7 +129,7 @@ export function AppRouter({ waitForMsw, sessionManager, telemetryService }: AppR return ( - + } @@ -133,7 +138,7 @@ export function AppRouter({ waitForMsw, sessionManager, telemetryService }: AppR onLoadPublicData={handleLoadPublicData} onLoadProtectedData={handleLoadProtectedData} isPublicDataLoaded={!!featureFlags} - isProtectedDataLoaded={!!sessionManager.getSession() && !!subscription} + isProtectedDataLoaded={isProtectedDataLoaded} onCompleteRegistrations={handleCompleteRegistrations} > {(routes, providerProps) => ( diff --git a/samples/endpoints/shell/src/useRefState.tsx b/samples/endpoints/shell/src/useRefState.tsx new file mode 100644 index 000000000..18a02e368 --- /dev/null +++ b/samples/endpoints/shell/src/useRefState.tsx @@ -0,0 +1,13 @@ +import { RefObject, useCallback, useRef } from "react"; + +export function useRefState(initialValue?: T): [RefObject, (newValue: T) => void] { + const valueRef = useRef(initialValue); + + const setValue = useCallback((newValue: T) => { + if (valueRef.current !== newValue) { + valueRef.current = newValue; + } + }, [valueRef]); + + return [valueRef, setValue]; +}