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.

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

fix: short circuit links and meta for routes that are not rendered due to errors #6107

Merged
merged 3 commits into from
Apr 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/link-meta-short-circuit.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/react": patch
---

short circuit links and meta for routes that are not rendered due to errors
60 changes: 60 additions & 0 deletions integration/link-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,9 @@ test.describe("route module link export", () => {
<li>
<Link to="/resources">Resource routes</Link>
</li>
<li>
<Link to="/parent/child">Errored child route</Link>
</li>
</ul>
</nav>
</div>
Expand Down Expand Up @@ -471,6 +474,42 @@ test.describe("route module link export", () => {
}

`,

"app/routes/parent.jsx": js`
import { Outlet } from "@remix-run/react";

export function links() {
return [
{ "data-test-id": "red" },
];
}

export default function Component() {
return <div data-test-id="/parent"><Outlet /></div>;
}

export function ErrorBoundary() {
return <h1 data-test-id="/parent:error-boundary">Error Boundary</h1>;
}
`,

"app/routes/parent.child.jsx": js`
import { Outlet } from "@remix-run/react";

export function loader() {
throw new Response(null, { status: 404 });
}

export function links() {
return [
{ "data-test-id": "blue" },
];
}

export default function Component() {
return <div data-test-id="/parent"><Outlet /></div>;
}
`,
},
});
appFixture = await createAppFixture(fixture);
Expand Down Expand Up @@ -511,6 +550,17 @@ test.describe("route module link export", () => {
expect(stylesheetResponses.length).toEqual(1);
});

test("does not render errored child route links", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/", true);
await page.click('a[href="/parent/child"]');
await page.waitForSelector('[data-test-id="/parent:error-boundary"]');
await page.waitForSelector('[data-test-id="red"]', { state: "attached" });
await page.waitForSelector('[data-test-id="blue"]', {
state: "detached",
});
});

test.describe("no js", () => {
test.use({ javaScriptEnabled: false });

Expand All @@ -534,6 +584,16 @@ test.describe("route module link export", () => {
let locator = page.locator("link[rel=preload][as=image]");
expect(await locator.getAttribute("imagesizes")).toBe("100vw");
});

test("does not render errored child route links", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/parent/child");
await page.waitForSelector('[data-test-id="/parent:error-boundary"]');
await page.waitForSelector('[data-test-id="red"]', { state: "attached" });
await page.waitForSelector('[data-test-id="blue"]', {
state: "detached",
});
});
});

test.describe("script imports", () => {
Expand Down
35 changes: 32 additions & 3 deletions packages/remix-react/components.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,14 @@ let fetcherSubmissionWarning =
*/
export function Links() {
let { manifest, routeModules } = useRemixContext();
let { matches } = useDataRouterStateContext();
let { errors, matches: routerMatches } = useDataRouterStateContext();

let matches = errors
? routerMatches.slice(
0,
routerMatches.findIndex((m) => errors![m.route.id]) + 1
)
: routerMatches;

let links = React.useMemo(
() => getLinksForMatches(matches, routeModules, manifest),
Expand Down Expand Up @@ -578,9 +585,20 @@ function PrefetchPageLinksImpl({
*/
function V1Meta() {
let { routeModules } = useRemixContext();
let { matches, loaderData } = useDataRouterStateContext();
let {
errors,
matches: routerMatches,
loaderData,
} = useDataRouterStateContext();
let location = useLocation();

let matches = errors
? routerMatches.slice(
0,
routerMatches.findIndex((m) => errors![m.route.id]) + 1
)
: routerMatches;

let meta: V1_HtmlMetaDescriptor = {};
let parentsData: { [routeId: string]: AppData } = {};

Expand Down Expand Up @@ -676,9 +694,20 @@ function V1Meta() {

function V2Meta() {
let { routeModules } = useRemixContext();
let { matches: _matches, loaderData } = useDataRouterStateContext();
let {
errors,
matches: routerMatches,
loaderData,
} = useDataRouterStateContext();
let location = useLocation();

let _matches = errors
? routerMatches.slice(
0,
routerMatches.findIndex((m) => errors![m.route.id]) + 1
)
: routerMatches;

let meta: V2_MetaDescriptor[] = [];
let leafMeta: V2_MetaDescriptor[] | null = null;
let matches: V2_MetaMatches = [];
Expand Down