Skip to content

Commit

Permalink
Remove internal discoverRoutes queue and make patch idempotent (#11977)
Browse files Browse the repository at this point in the history
  • Loading branch information
@brophdawg11
brophdawg11 authored Sep 10, 2024
1 parent 8c9e2b6 commit 2dd13c6
Show file tree
Hide file tree
Showing 4 changed files with 155 additions and 138 deletions.
5 changes: 5 additions & 0 deletions .changeset/unlucky-keys-collect.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/router": patch
---

Remove internal `discoveredRoutes` FIFO queue from `unstable_patchRoutesOnNavigation`
82 changes: 82 additions & 0 deletions docs/routers/create-browser-router.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,88 @@ let router = createBrowserRouter(
);
```
### A note on routes with parameters
Because React Router uses ranked routes to find the best match for a given path, there is an interesting ambiguity introduced when only a partial route tree is known at any given point in time. If we match a fully static route such as `path: "/about/contact-us"` then we know we've found the right match since it's composed entirely of static URL segments, and thus we do not need to bother asking for any other potentially higher-scoring routes.
However, routes with parameters (dynamic or splat) can't make this assumption because there might be a not-yet-discovered route tht scores higher. Consider a full route tree such as:
```js
// Assume this is the full route tree for your app
const routes = [
{
path: "/",
Component: Home,
},
{
id: "blog",
path: "/blog",
Component: BlogLayout,
children: [
{ path: "new", Component: NewPost },
{ path: ":slug", Component: BlogPost },
],
},
];
```
And then assume we want to use `patchRoutesOnNavigation` to fill this in as the user navigates around:
```js
// Start with only the index route
const router = createBrowserRouter(
[
{
path: "/",
Component: Home,
},
],
{
patchRoutesOnNavigation({ path, patch }) {
if (path === "/blog/new") {
patch("blog", [
{
path: "new",
Component: NewPost,
},
]);
} else if (path.startsWith("/blog")) {
patch("blog", [
{
path: ":slug",
Component: BlogPost,
},
]);
}
},
}
);
```
If the user were to a blog post first (i.e., `/blog/my-post`) we would patch in the `:slug` route. Then if the user navigated to `/blog/new` to write a new post, we'd match `/blog/:slug` but it wouldn't be the _right_ match! We need to call `patchRoutesOnNavigation` just in case there exists a higher-scoring route we've not yet discovered, which in this case there is.
So, anytime React Router matches a path that contains at least one param, it will call `patchRoutesOnNavigation` and match routes again just to confirm it has found the best match.
If your `patchRoutesOnNavigation` implementation is expensive or making side-effect `fetch` calls to a backend server, you may want to consider tracking previously seen routes to avoid over-fetching in cases where you know the proper route has already been found. This can usually be as simple as maintaining a small cache of prior `path` values for which you've already patched in the right routes:
```js
let discoveredRoutes = new Set();

const router = createBrowserRouter(routes, {
patchRoutesOnNavigation({ path, patch }) {
if (discoveredRoutes.has(path)) {
// We've seen this before so nothing to patch in and we can let the router
// use the routes it already knows about
return;
}

discoveredRoutes.add(path);

// ... patch routes in accordingly
},
});
```
## `opts.window`
Useful for environments like browser devtool plugins or testing to use a different window than the global `window`.
Expand Down
141 changes: 41 additions & 100 deletions packages/router/__tests__/lazy-discovery-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1259,134 +1259,75 @@ describe("Lazy Route Discovery (Fog of War)", () => {
unsubscribe();
});

it('does not re-call for previously called "good" paths', async () => {
it("does not re-patch previously patched routes", async () => {
let count = 0;
router = createRouter({
history: createMemoryHistory(),
routes: [
{
path: "/",
},
{
id: "param",
path: ":param",
},
],
async patchRoutesOnNavigation() {
async patchRoutesOnNavigation({ patch }) {
count++;
patch(null, [
{
id: "param",
path: ":param",
},
]);
await tick();
// Nothing to patch - there is no better static route in this case
},
});

await router.navigate("/whatever");
expect(count).toBe(1);
expect(router.state.location.pathname).toBe("/whatever");
await router.navigate("/a");
expect(router.state.location.pathname).toBe("/a");
expect(router.state.matches.map((m) => m.route.id)).toEqual(["param"]);

await router.navigate("/");
expect(count).toBe(1);
expect(router.state.location.pathname).toBe("/");

await router.navigate("/whatever");
expect(count).toBe(1); // Not called again
expect(router.state.location.pathname).toBe("/whatever");
expect(router.state.matches.map((m) => m.route.id)).toEqual(["param"]);
});

it("does not re-call for previously called 404 paths", async () => {
let count = 0;
router = createRouter({
history: createMemoryHistory(),
routes: [
expect(router.routes).toMatchInlineSnapshot(`
[
{
id: "index",
path: "/",
"children": undefined,
"hasErrorBoundary": false,
"id": "0",
"path": "/",
},
{
id: "static",
path: "static",
"children": undefined,
"hasErrorBoundary": false,
"id": "param",
"path": ":param",
},
],
async patchRoutesOnNavigation() {
count++;
},
});

await router.navigate("/junk");
expect(count).toBe(1);
expect(router.state.location.pathname).toBe("/junk");
expect(router.state.errors?.index).toEqual(
new ErrorResponseImpl(
404,
"Not Found",
new Error('No route matches URL "/junk"'),
true
)
);
]
`);

await router.navigate("/");
expect(count).toBe(1);
expect(router.state.location.pathname).toBe("/");
expect(router.state.errors).toBeNull();

await router.navigate("/junk");
expect(count).toBe(1);
expect(router.state.location.pathname).toBe("/junk");
expect(router.state.errors?.index).toEqual(
new ErrorResponseImpl(
404,
"Not Found",
new Error('No route matches URL "/junk"'),
true
)
);
});

it("caps internal fifo queue at 1000 paths", async () => {
let count = 0;
router = createRouter({
history: createMemoryHistory(),
routes: [
await router.navigate("/b");
expect(router.state.location.pathname).toBe("/b");
expect(router.state.matches.map((m) => m.route.id)).toEqual(["param"]);
expect(router.state.errors).toBeNull();
// Called again
expect(count).toBe(2);
// But not patched again
expect(router.routes).toMatchInlineSnapshot(`
[
{
path: "/",
"children": undefined,
"hasErrorBoundary": false,
"id": "0",
"path": "/",
},
{
id: "param",
path: ":param",
"children": undefined,
"hasErrorBoundary": false,
"id": "param",
"path": ":param",
},
],
async patchRoutesOnNavigation() {
count++;
// Nothing to patch - there is no better static route in this case
},
});

// Fill it up with 1000 paths
for (let i = 1; i <= 1000; i++) {
await router.navigate(`/path-${i}`);
expect(count).toBe(i);
expect(router.state.location.pathname).toBe(`/path-${i}`);

await router.navigate("/");
expect(count).toBe(i);
expect(router.state.location.pathname).toBe("/");
}

// Don't call patchRoutesOnNavigation since this is the first item in the queue
await router.navigate(`/path-1`);
expect(count).toBe(1000);
expect(router.state.location.pathname).toBe(`/path-1`);

// Call patchRoutesOnNavigation and evict the first item
await router.navigate(`/path-1001`);
expect(count).toBe(1001);
expect(router.state.location.pathname).toBe(`/path-1001`);

// Call patchRoutesOnNavigation since this item was evicted
await router.navigate(`/path-1`);
expect(count).toBe(1002);
expect(router.state.location.pathname).toBe(`/path-1`);
]
`);
});

describe("errors", () => {
Expand Down
65 changes: 27 additions & 38 deletions packages/router/router.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -814,10 +814,6 @@ export function createRouter(init: RouterInit): Router {
let unlistenHistory: (() => void) | null = null;
// Externally-provided functions to call on all state changes
let subscribers = new Set<RouterSubscriber>();
// FIFO queue of previously discovered routes to prevent re-calling on
// subsequent navigations to the same path
let discoveredRoutesMaxSize = 1000;
let discoveredRoutes = new Set<string>();
// Externally-provided object to hold scroll restoration locations during routing
let savedScrollPositions: Record<string, number> | null = null;
// Externally-provided function to get scroll restoration keys
Expand Down Expand Up @@ -3243,13 +3239,6 @@ export function createRouter(init: RouterInit): Router {
pathname: string
): { active: boolean; matches: AgnosticDataRouteMatch[] | null } {
if (patchRoutesOnNavigationImpl) {
// Don't bother re-calling patchRouteOnMiss for a path we've already
// processed. the last execution would have patched the route tree
// accordingly so `matches` here are already accurate.
if (discoveredRoutes.has(pathname)) {
return { active: false, matches };
}

if (!matches) {
let fogMatches = matchRoutesImpl<AgnosticDataRouteObject>(
routesToUse,
Expand Down Expand Up @@ -3333,7 +3322,6 @@ export function createRouter(init: RouterInit): Router {

let newMatches = matchRoutes(routesToUse, pathname, basename);
if (newMatches) {
addToFifoQueue(pathname, discoveredRoutes);
return { type: "success", matches: newMatches };
}

Expand All @@ -3352,22 +3340,13 @@ export function createRouter(init: RouterInit): Router {
(m, i) => m.route.id === newPartialMatches![i].route.id
))
) {
addToFifoQueue(pathname, discoveredRoutes);
return { type: "success", matches: null };
}

partialMatches = newPartialMatches;
}
}

function addToFifoQueue(path: string, queue: Set<string>) {
if (queue.size >= discoveredRoutesMaxSize) {
let first = queue.values().next().value;
queue.delete(first);
}
queue.add(path);
}

function _internalSetRoutes(newRoutes: AgnosticDataRouteObject[]) {
manifest = {};
inFlightDataRoutes = convertRoutesToDataRoutes(
Expand Down Expand Up @@ -4661,32 +4640,42 @@ function patchRoutesImpl(
manifest: RouteManifest,
mapRouteProperties: MapRoutePropertiesFunction
) {
let childrenToPatch: AgnosticDataRouteObject[];
if (routeId) {
let route = manifest[routeId];
invariant(
route,
`No route found to patch children into: routeId = ${routeId}`
);
let dataChildren = convertRoutesToDataRoutes(
children,
mapRouteProperties,
[routeId, "patch", String(route.children?.length || "0")],
manifest
);
if (route.children) {
route.children.push(...dataChildren);
} else {
route.children = dataChildren;
if (!route.children) {
route.children = [];
}
childrenToPatch = route.children;
} else {
let dataChildren = convertRoutesToDataRoutes(
children,
mapRouteProperties,
["patch", String(routesToUse.length || "0")],
manifest
);
routesToUse.push(...dataChildren);
childrenToPatch = routesToUse;
}

// Don't patch in routes we already know about so that `patch` is idempotent
// to simplify user-land code. This is useful because we re-call the
// `patchRoutesOnNavigation` function for matched routes with params.
let uniqueChildren = children.filter(
(a) =>
!childrenToPatch.some(
(b) =>
a.index === b.index &&
a.path === b.path &&
a.caseSensitive === b.caseSensitive
)
);

let newRoutes = convertRoutesToDataRoutes(
uniqueChildren,
mapRouteProperties,
[routeId || "_", "patch", String(childrenToPatch?.length || "0")],
manifest
);

childrenToPatch.push(...newRoutes);
}

/**
Expand Down

0 comments on commit 2dd13c6

Please sign in to comment.