Skip to content

Commit

Permalink
Call patchRoutesOnMiss when matching slug routes in case there exist …
Browse files Browse the repository at this point in the history
…higher scoring static routes (#11883)
  • Loading branch information
brophdawg11 authored Aug 13, 2024
1 parent de4e366 commit bd4ac8e
Show file tree
Hide file tree
Showing 3 changed files with 365 additions and 42 deletions.
7 changes: 7 additions & 0 deletions .changeset/cyan-bobcats-notice.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@remix-run/router": patch
---

Fog of War: Update `unstable_patchRoutesOnMiss` logic so that we call the method when we match routes with dynamic param or splat segments in case there exists a higher-scoring static route that we've not yet discovered.

- We also now leverage an internal FIFO queue of previous paths we've already called `unstable_patchRouteOnMiss` against so that we don't re-call on subsequent navigations to the same path
326 changes: 326 additions & 0 deletions packages/router/__tests__/lazy-discovery-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,121 @@ describe("Lazy Route Discovery (Fog of War)", () => {
]);
});

it("de-prioritizes dynamic param routes in favor of looking for better async matches", async () => {
router = createRouter({
history: createMemoryHistory(),
routes: [
{
path: "/",
},
{
id: "slug",
path: "/:slug",
},
],
async unstable_patchRoutesOnMiss({ patch }) {
await tick();
patch(null, [
{
id: "static",
path: "/static",
},
]);
},
});

await router.navigate("/static");
expect(router.state.location.pathname).toBe("/static");
expect(router.state.matches.map((m) => m.route.id)).toEqual(["static"]);
});

it("de-prioritizes dynamic param routes in favor of looking for better async matches (product/:slug)", async () => {
router = createRouter({
history: createMemoryHistory(),
routes: [
{
path: "/",
},
{
id: "slug",
path: "/product/:slug",
},
],
async unstable_patchRoutesOnMiss({ patch }) {
await tick();
patch(null, [
{
id: "static",
path: "/product/static",
},
]);
},
});

await router.navigate("/product/static");
expect(router.state.location.pathname).toBe("/product/static");
expect(router.state.matches.map((m) => m.route.id)).toEqual(["static"]);
});

it("de-prioritizes dynamic param routes in favor of looking for better async matches (child route)", async () => {
router = createRouter({
history: createMemoryHistory(),
routes: [
{
path: "/",
},
{
id: "product",
path: "/product",
children: [
{
id: "slug",
path: ":slug",
},
],
},
],
async unstable_patchRoutesOnMiss({ patch }) {
await tick();
patch("product", [
{
id: "static",
path: "static",
},
]);
},
});

await router.navigate("/product/static");
expect(router.state.location.pathname).toBe("/product/static");
expect(router.state.matches.map((m) => m.route.id)).toEqual([
"product",
"static",
]);
});

it("matches dynamic params when other paths don't pan out", async () => {
router = createRouter({
history: createMemoryHistory(),
routes: [
{
path: "/",
},
{
id: "slug",
path: "/:slug",
},
],
async unstable_patchRoutesOnMiss({ matches, patch }) {
await tick();
},
});

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

it("de-prioritizes splat routes in favor of looking for better async matches", async () => {
router = createRouter({
history: createMemoryHistory(),
Expand Down Expand Up @@ -569,6 +684,43 @@ describe("Lazy Route Discovery (Fog of War)", () => {
expect(router.state.matches.map((m) => m.route.id)).toEqual(["static"]);
});

it("de-prioritizes splat routes in favor of looking for better async matches (child route)", async () => {
router = createRouter({
history: createMemoryHistory(),
routes: [
{
path: "/",
},
{
id: "product",
path: "/product",
children: [
{
id: "splat",
path: "*",
},
],
},
],
async unstable_patchRoutesOnMiss({ patch }) {
await tick();
patch("product", [
{
id: "static",
path: "static",
},
]);
},
});

await router.navigate("/product/static");
expect(router.state.location.pathname).toBe("/product/static");
expect(router.state.matches.map((m) => m.route.id)).toEqual([
"product",
"static",
]);
});

it("matches splats when other paths don't pan out", async () => {
router = createRouter({
history: createMemoryHistory(),
Expand Down Expand Up @@ -603,6 +755,50 @@ describe("Lazy Route Discovery (Fog of War)", () => {
expect(router.state.matches.map((m) => m.route.id)).toEqual(["splat"]);
});

it("recurses unstable_patchRoutesOnMiss until a match is found", async () => {
let count = 0;
router = createRouter({
history: createMemoryHistory(),
routes: [
{
path: "/",
},
{
id: "a",
path: "a",
},
],
async unstable_patchRoutesOnMiss({ matches, patch }) {
await tick();
count++;
if (last(matches).route.id === "a") {
patch("a", [
{
id: "b",
path: "b",
},
]);
} else if (last(matches).route.id === "b") {
patch("b", [
{
id: "c",
path: "c",
},
]);
}
},
});

await router.navigate("/a/b/c");
expect(router.state.location.pathname).toBe("/a/b/c");
expect(router.state.matches.map((m) => m.route.id)).toEqual([
"a",
"b",
"c",
]);
expect(count).toBe(2);
});

it("discovers routes during initial hydration", async () => {
let childrenDfd = createDeferred<AgnosticDataRouteObject[]>();
let loaderDfd = createDeferred();
Expand Down Expand Up @@ -1063,6 +1259,136 @@ describe("Lazy Route Discovery (Fog of War)", () => {
unsubscribe();
});

it('does not re-call for previously called "good" paths', async () => {
let count = 0;
router = createRouter({
history: createMemoryHistory(),
routes: [
{
path: "/",
},
{
id: "param",
path: ":param",
},
],
async unstable_patchRoutesOnMiss() {
count++;
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");
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: [
{
id: "index",
path: "/",
},
{
id: "static",
path: "static",
},
],
async unstable_patchRoutesOnMiss() {
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: [
{
path: "/",
},
{
id: "param",
path: ":param",
},
],
async unstable_patchRoutesOnMiss() {
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 patchRoutesOnMiss 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 patchRoutesOnMiss and evict the first item
await router.navigate(`/path-1001`);
expect(count).toBe(1001);
expect(router.state.location.pathname).toBe(`/path-1001`);

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

describe("errors", () => {
it("lazy 404s (GET navigation)", async () => {
let childrenDfd = createDeferred<AgnosticDataRouteObject[]>();
Expand Down
Loading

0 comments on commit bd4ac8e

Please sign in to comment.