Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle pathless/index route children for initially rendered ancestors in Fog of War #9624

Merged
merged 2 commits into from
Jun 17, 2024
Merged

Handle pathless/index route children for initially rendered ancestors in Fog of War #9624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6188f77
Handle index routes at all ancestors in Fog of War
brophdawg11 Jun 17, 2024
5055faf
Change approach to use matching
brophdawg11 Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .changeset/eleven-snakes-end.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
"@remix-run/react": patch
---

Fix fog of war issue with routing back to homepage from deep SSR page
[REMOVE] Fix fog of war issue with routing back to homepage from deep SSR page
5 changes: 5 additions & 0 deletions .changeset/popular-numbers-hang.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/react": patch
---

[REMOVE] Properly handle pathless children of ancestor routes in FOW
257 changes: 255 additions & 2 deletions integration/fog-of-war-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ test.describe("Fog of War", () => {
).toEqual(["root", "routes/_index", "routes/a", "routes/a.b"]);
});

test("prefetches `routes/_index` when SSR-ing a deep route for client-side links to '/'", async ({
test("prefetches root index child when SSR-ing a deep route", async ({
page,
}) => {
let fixture = await createFixture({
Expand Down Expand Up @@ -414,15 +414,268 @@ test.describe("Fog of War", () => {
let appFixture = await createAppFixture(fixture);
let app = new PlaywrightFixture(appFixture, page);

let manifestRequests: string[] = [];
page.on("request", (req) => {
if (req.url().includes("/__manifest")) {
manifestRequests.push(req.url());
}
});

await app.goto("/deep", true);
expect(
await page.evaluate(() =>
Object.keys((window as any).__remixManifest.routes)
)
).toEqual(["routes/_index", "root", "routes/deep"]);
).toEqual(["root", "routes/deep", "routes/_index"]);

// Without pre-loading the index, we'd "match" `/` to just the root route
// client side and never fetch the `routes/_index` route
await app.clickLink("/");
await page.waitForSelector("#index");
expect(await app.getHtml("#index")).toMatch(`Index`);

expect(manifestRequests.length).toBe(0);
});

test("prefetches ancestor index children when SSR-ing a deep route", async ({
page,
}) => {
let fixture = await createFixture({
config: {
future: {
unstable_fogOfWar: true,
},
},
files: {
"app/root.tsx": js`
import { Link, Outlet, Scripts } from "@remix-run/react";

export default function Root() {
return (
<html lang="en">
<head></head>
<body >
<nav>
<Link to="/parent" discover="none">Parent Index</Link>
<Link to="/parent/child" discover="none">Child Index</Link>
</nav>
<Outlet />
<Scripts />
</body>
</html>
);
}
`,

"app/routes/_index.tsx": js`
export default function Index() {
return <h1 id="index">Index</h1>
}
`,
"app/routes/parent.tsx": js`
import { Outlet } from "@remix-run/react";
export default function Component() {
return (
<>
<h1 id="parent">Parent</h1>
<Outlet />
</>
)
}
`,
"app/routes/parent._index.tsx": js`
export default function Component() {
return <h2 id="parent-index">Parent Index</h2>;
}
`,
"app/routes/parent.child.tsx": js`
import { Outlet } from "@remix-run/react";
export default function Component() {
return (
<>
<h2 id="child">Child</h2>
<Outlet />
</>
)
}
`,
"app/routes/parent.child._index.tsx": js`
export default function Component() {
return <h3 id="child-index">Child Index</h3>;
}
`,
"app/routes/parent.child.grandchild.tsx": js`
export default function Component() {
return <h3 id="grandchild">Grandchild</h3>;
}
`,
},
});
let appFixture = await createAppFixture(fixture);
let app = new PlaywrightFixture(appFixture, page);

let manifestRequests: string[] = [];
page.on("request", (req) => {
if (req.url().includes("/__manifest")) {
manifestRequests.push(req.url());
}
});

await app.goto("/parent/child/grandchild", true);
expect(
await page.evaluate(() =>
Object.keys((window as any).__remixManifest.routes)
)
).toEqual([
"root",
"routes/parent",
"routes/parent.child",
"routes/parent.child.grandchild",
"routes/_index",
"routes/parent.child._index",
"routes/parent._index",
]);

// Without pre-loading the index, we'd "match" `/parent/child` to just the
// parent and child routes client side and never fetch the
// `routes/parent.child._index` route
await app.clickLink("/parent/child");
await page.waitForSelector("#child-index");
expect(await app.getHtml("#parent")).toMatch("Parent");
expect(await app.getHtml("#child")).toMatch("Child");
expect(await app.getHtml("#child-index")).toMatch(`Child Index`);

await app.clickLink("/parent");
await page.waitForSelector("#parent-index");
expect(await app.getHtml("#parent")).toMatch(`Parent`);
expect(await app.getHtml("#parent-index")).toMatch(`Parent Index`);

expect(manifestRequests.length).toBe(0);
});

test("prefetches ancestor pathless children when SSR-ing a deep route", async ({
page,
}) => {
let fixture = await createFixture({
config: {
future: {
unstable_fogOfWar: true,
},
},
files: {
"app/root.tsx": js`
import { Link, Outlet, Scripts } from "@remix-run/react";

export default function Root() {
return (
<html lang="en">
<head></head>
<body >
<nav>
<Link to="/parent" discover="none">Parent Index</Link>
<Link to="/parent/child2" discover="none">Child2</Link>
</nav>
<Outlet />
<Scripts />
</body>
</html>
);
}
`,

"app/routes/_index.tsx": js`
export default function Index() {
return <h1 id="index">Index</h1>
}
`,
"app/routes/parent.tsx": js`
import { Outlet } from "@remix-run/react";
export default function Component() {
return (
<>
<h1 id="parent">Parent</h1>
<Outlet />
</>
)
}
`,
"app/routes/parent.child.tsx": js`
export default function Component() {
return <h2 id="child">Child</h2>;
}
`,
"app/routes/parent._a.tsx": js`
import { Outlet } from '@remix-run/react';
export default function Component() {
return <div id="a"><Outlet/></div>;
}
`,
"app/routes/parent._a._b._index.tsx": js`
export default function Component() {
return <h2 id="parent-index">Parent Pathless Index</h2>;
}
`,
"app/routes/parent._a._b.tsx": js`
import { Outlet } from '@remix-run/react';
export default function Component() {
return <div id="b"><Outlet/></div>;
}
`,
"app/routes/parent._a._b.child2.tsx": js`
export default function Component() {
return <h2 id="child2">Child 2</h2>;
}
`,
},
});
let appFixture = await createAppFixture(fixture);
let app = new PlaywrightFixture(appFixture, page);

let manifestRequests: string[] = [];
page.on("request", (req) => {
if (req.url().includes("/__manifest")) {
manifestRequests.push(req.url());
}
});

await app.goto("/parent/child", true);
expect(await app.getHtml("#child")).toMatch("Child");
expect(await page.$("#a")).toBeNull();
expect(await page.$("#b")).toBeNull();

expect(
await page.evaluate(() =>
Object.keys((window as any).__remixManifest.routes)
)
).toEqual([
"root",
"routes/parent",
"routes/parent.child",
"routes/_index",
"routes/parent._a",
"routes/parent._a._b",
"routes/parent._a._b._index",
]);

// Without pre-loading the index, we'd "match" `/parent` to just the
// parent route client side and never fetch the children pathless/index routes
await app.clickLink("/parent");
await page.waitForSelector("#parent-index");
expect(await page.$("#a")).not.toBeNull();
expect(await page.$("#b")).not.toBeNull();
expect(await app.getHtml("#parent")).toMatch("Parent");
expect(await app.getHtml("#parent-index")).toMatch("Parent Pathless Index");
expect(manifestRequests.length).toBe(0);

// This will require a new fetch for the child2 portion
await app.clickLink("/parent/child2");
await page.waitForSelector("#child2");
expect(await app.getHtml("#parent")).toMatch(`Parent`);
expect(await app.getHtml("#child2")).toMatch(`Child 2`);
expect(manifestRequests).toEqual([
expect.stringMatching(
/\/__manifest\?version=[a-z0-9]{8}&paths=%2Fparent%2Fchild2/
),
]);
});
});
4 changes: 2 additions & 2 deletions packages/remix-react/components.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ export function Scripts(props: ScriptProps) {
future,
renderMeta,
} = useRemixContext();
let { static: isStatic, staticContext } = useDataRouterContext();
let { router, static: isStatic, staticContext } = useDataRouterContext();
let { matches: routerMatches } = useDataRouterStateContext();
let enableFogOfWar = isFogOfWarEnabled(future, isSpaMode);

Expand Down Expand Up @@ -881,7 +881,7 @@ ${
enableFogOfWar
? // Inline a minimal manifest with the SSR matches
`window.__remixManifest = ${JSON.stringify(
getPartialManifest(manifest, matches),
getPartialManifest(manifest, router),
null,
2
)};`
Expand Down
51 changes: 29 additions & 22 deletions packages/remix-react/fog-of-war.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,31 +31,38 @@ export function isFogOfWarEnabled(future: FutureConfig, isSpaMode: boolean) {
return future.unstable_fogOfWar === true && !isSpaMode;
}

export function getPartialManifest(manifest: AssetsManifest, matches: any[]) {
let rootIndexRoute = Object.values(manifest.routes).find(
(r) => r.parentId === "root" && r.index === true
export function getPartialManifest(manifest: AssetsManifest, router: Router) {
// Start with our matches for this pathname
let routeIds = new Set(router.state.matches.map((m) => m.route.id));

let segments = router.state.location.pathname.split("/").filter(Boolean);
let paths: string[] = ["/"];

// We've already matched to the last segment
segments.pop();

// Traverse each path for our parents and match in case they have pathless/index
// children we need to include in the initial manifest
while (segments.length > 0) {
paths.push(`/${segments.join("/")}`);
segments.pop();
}

paths.forEach((path) => {
let matches = matchRoutes(router.routes, path, router.basename);
if (matches) {
matches.forEach((m) => routeIds.add(m.route.id));
}
});

let initialRoutes = [...routeIds].reduce(
(acc, id) => Object.assign(acc, { [id]: manifest.routes[id] }),
{}
);
let matchesContainsIndex =
rootIndexRoute && !matches.some((m) => m.route.id === rootIndexRoute!.id);

return {
...manifest,
routes: {
// Include the root index route if we enter on a different route, otherwise
// we can get a false positive when client-side matching on a link back to
// `/` since we will match the root route
...(matchesContainsIndex
? {
[rootIndexRoute!.id]: rootIndexRoute,
}
: {}),
...matches.reduce(
(acc, m) =>
Object.assign(acc, {
[m.route.id]: manifest.routes[m.route.id],
}),
{}
),
},
routes: initialRoutes,
};
}

Expand Down