Skip to content

Commit

Permalink
Remove internal discoverRoutes queue and make patch idempotent (#11978)
Browse files Browse the repository at this point in the history
  • Loading branch information
@brophdawg11
brophdawg11 authored Sep 10, 2024
1 parent 0fc83f8 commit aa37dd1
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 138 deletions.
141 changes: 41 additions & 100 deletions packages/react-router/__tests__/router/lazy-discovery-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1202,134 +1202,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/react-router/lib/router/router.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -819,10 +819,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 @@ -3146,13 +3142,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 @@ -3236,7 +3225,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 @@ -3255,22 +3243,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 @@ -4498,32 +4477,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 aa37dd1

Please sign in to comment.