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

Updates to static handler for Remix integration #9511

Merged
merged 7 commits into from Nov 1, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/proud-timers-tickle.md
@@ -0,0 +1,5 @@
---
"@remix-run/router": patch
---

fix: actions/loaders returning undefined should throw an error
10 changes: 5 additions & 5 deletions packages/react-router-dom/__tests__/nav-link-active-test.tsx
Expand Up @@ -458,7 +458,7 @@ describe("NavLink using a data router", () => {
fireEvent.click(screen.getByText("Link to Bar"));
expect(screen.getByText("Link to Bar").className).toBe("pending");

dfd.resolve();
dfd.resolve(null);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests used to return undefined, so make them return null now

await waitFor(() => screen.getByText("Bar page"));
expect(screen.getByText("Link to Bar").className).toBe("active");
});
Expand Down Expand Up @@ -511,7 +511,7 @@ describe("NavLink using a data router", () => {
"some-pending-classname"
);

dfd.resolve();
dfd.resolve(null);
await waitFor(() => screen.getByText("Bar page"));
expect(screen.getByText("Link to Bar").className).toBe(
"some-active-classname"
Expand Down Expand Up @@ -566,7 +566,7 @@ describe("NavLink using a data router", () => {
"lowercase"
);

dfd.resolve();
dfd.resolve(null);
await waitFor(() => screen.getByText("Bar page"));
expect(screen.getByText("Link to Bar").style.textTransform).toBe(
"uppercase"
Expand Down Expand Up @@ -616,7 +616,7 @@ describe("NavLink using a data router", () => {
fireEvent.click(screen.getByText("Link to Bar (idle)"));
expect(screen.getByText("Link to Bar (loading...)")).toBeDefined();

dfd.resolve();
dfd.resolve(null);
await waitFor(() => screen.getByText("Bar page"));
expect(screen.getByText("Link to Bar (current)")).toBeDefined();
});
Expand Down Expand Up @@ -657,7 +657,7 @@ describe("NavLink using a data router", () => {
fireEvent.click(screen.getByText("Link to Baz"));
expect(screen.getByText("Link to Bar").className).toBe("");

dfd.resolve();
dfd.resolve(null);
await waitFor(() => screen.getByText("Baz page"));
expect(screen.getByText("Link to Bar").className).toBe("");
});
Expand Down
Expand Up @@ -2713,7 +2713,7 @@ describe("<DataMemoryRouter>", () => {
expect(getAwaitRenderCount()).toBe(3);

// complete /baz navigation
bazDefer.resolve();
bazDefer.resolve(null);
await waitFor(() => screen.getByText("Baz"));
expect(getHtml(container)).toMatchInlineSnapshot(`
"<div
Expand Down
242 changes: 160 additions & 82 deletions packages/router/__tests__/router-test.ts
Expand Up @@ -32,7 +32,11 @@ import type {
AgnosticRouteObject,
TrackedPromise,
} from "../utils";
import { AbortedDeferredError, stripBasename } from "../utils";
import {
AbortedDeferredError,
isErrorWithStatus,
stripBasename,
} from "../utils";

///////////////////////////////////////////////////////////////////////////////
//#region Types and Utils
Expand Down Expand Up @@ -5069,6 +5073,61 @@ describe("a router", () => {

router.dispose();
});

it("throws an error if actions/loaders return undefined", async () => {
let t = setup({
routes: [
{
index: true,
},
{
id: "path",
path: "/path",
loader: true,
action: true,
},
],
});

let nav1 = await t.navigate("/path");
await nav1.loaders.path.resolve(undefined);
expect(t.router.state).toMatchObject({
location: {
pathname: "/path",
},
errors: {
path: new Error(
'You defined a loader for route "path" but didn\'t return anything ' +
"from your `loader` function. Please return a value or `null`."
),
},
});

await t.navigate("/");
expect(t.router.state).toMatchObject({
location: {
pathname: "/",
},
errors: {},
});

let nav3 = await t.navigate("/path", {
formMethod: "post",
formData: createFormData({}),
});
await nav3.actions.path.resolve(undefined);
expect(t.router.state).toMatchObject({
location: {
pathname: "/path",
},
errors: {
path: new Error(
'You defined an action for route "path" but didn\'t return anything ' +
"from your `action` function. Please return a value or `null`."
),
},
});
});
});

describe("redirects", () => {
Expand Down Expand Up @@ -11066,6 +11125,35 @@ describe("a router", () => {
expect(data).toBe("");
});

it("should error if an action/loader returns undefined", async () => {
let T = setupFlexRouteTest();
let data;

try {
data = await T.resolveLoader(undefined);
} catch (e) {
data = e;
}
expect(data).toEqual(
new Error(
'You defined a loader for route "flex" but didn\'t return anything ' +
"from your `loader` function. Please return a value or `null`."
)
);

try {
data = await T.resolveAction(undefined);
} catch (e) {
data = e;
}
expect(data).toEqual(
new Error(
'You defined an action for route "flex" but didn\'t return anything ' +
"from your `action` function. Please return a value or `null`."
)
);
});

it("should handle relative redirect responses (loader)", async () => {
let { queryRoute } = createStaticHandler([
{
Expand Down Expand Up @@ -11214,7 +11302,7 @@ describe("a router", () => {
);
});

it("should handle not found routes with a 404 Response", async () => {
describe("Errors with Status Codes", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

View this section with whitespace hidden, but now instead of returning responses we return a new ErrorWithStatus instance and we align the error messages with what Remix uses

/* eslint-disable jest/no-conditional-expect */
let { queryRoute } = createStaticHandler([
{
Expand All @@ -11223,93 +11311,83 @@ describe("a router", () => {
},
]);

try {
await queryRoute(createRequest("/junk"));
expect(false).toBe(true);
} catch (data) {
expect(data instanceof Response).toBe(true);
expect(data.status).toBe(404);
expect(data.statusText).toBe("Not Found");
expect(data.headers.get("X-Remix-Router-Error")).toBe("yes");
}

try {
await queryRoute(createRequest("/"), "junk");
expect(false).toBe(true);
} catch (data) {
expect(data instanceof Response).toBe(true);
expect(data.status).toBe(404);
expect(data.statusText).toBe("Not Found");
expect(data.headers.get("X-Remix-Router-Error")).toBe("yes");
}
it("should handle not found paths with a 404 Response", async () => {
try {
await queryRoute(createRequest("/junk"));
expect(false).toBe(true);
} catch (data) {
expect(isErrorWithStatus(data)).toBe(true);
expect(data.status).toBe(404);
expect(data.message).toBe('No route matches URL "/junk"');
}

try {
await queryRoute(createSubmitRequest("/junk"));
expect(false).toBe(true);
} catch (data) {
expect(data instanceof Response).toBe(true);
expect(data.status).toBe(404);
expect(data.statusText).toBe("Not Found");
expect(data.headers.get("X-Remix-Router-Error")).toBe("yes");
}
try {
await queryRoute(createSubmitRequest("/junk"));
expect(false).toBe(true);
} catch (data) {
expect(isErrorWithStatus(data)).toBe(true);
expect(data.status).toBe(404);
expect(data.message).toBe('No route matches URL "/junk"');
}
});

try {
await queryRoute(createSubmitRequest("/"), "junk");
expect(false).toBe(true);
} catch (data) {
expect(data instanceof Response).toBe(true);
expect(data.status).toBe(404);
expect(data.statusText).toBe("Not Found");
expect(data.headers.get("X-Remix-Router-Error")).toBe("yes");
}
it("should handle not found routeIds with a 403 Response", async () => {
try {
await queryRoute(createRequest("/"), "junk");
expect(false).toBe(true);
} catch (data) {
expect(isErrorWithStatus(data)).toBe(true);
expect(data.status).toBe(403);
expect(data.message).toBe('Route "junk" does not match URL "/"');
}

/* eslint-enable jest/no-conditional-expect */
});
try {
await queryRoute(createSubmitRequest("/"), "junk");
expect(false).toBe(true);
} catch (data) {
expect(isErrorWithStatus(data)).toBe(true);
expect(data.status).toBe(403);
expect(data.message).toBe('Route "junk" does not match URL "/"');
}
});

it("should handle not found action submissions with a 405 Response", async () => {
/* eslint-disable jest/no-conditional-expect */
let { queryRoute } = createStaticHandler([
{
id: "root",
path: "/",
},
]);
it("should handle not found action/loader submissions with a 405 Response", async () => {
try {
await queryRoute(createRequest("/"), "root");
expect(false).toBe(true);
} catch (data) {
expect(isErrorWithStatus(data)).toBe(true);
expect(data.status).toBe(405);
expect(data.message).toBe(
'You made a GET request to "/" but did not provide a `loader` ' +
'for route "root", so there is no way to handle the request.'
);
}

try {
await queryRoute(createSubmitRequest("/"), "root");
expect(false).toBe(true);
} catch (data) {
expect(data instanceof Response).toBe(true);
expect(data.status).toBe(405);
expect(data.statusText).toBe("Method Not Allowed");
expect(data.headers.get("X-Remix-Router-Error")).toBe("yes");
expect(await data.text()).toBe("");
}
/* eslint-enable jest/no-conditional-expect */
});
try {
await queryRoute(createSubmitRequest("/"), "root");
expect(false).toBe(true);
} catch (data) {
expect(isErrorWithStatus(data)).toBe(true);
expect(data.status).toBe(405);
expect(data.message).toBe(
'You made a POST request to "/" but did not provide an `action` ' +
'for route "root", so there is no way to handle the request.'
);
}
});

it("should handle unsupported methods with a 405 Response", async () => {
/* eslint-disable jest/no-conditional-expect */
let { queryRoute } = createStaticHandler([
{
id: "root",
path: "/",
},
]);
it("should handle unsupported methods with a 405 Response", async () => {
try {
await queryRoute(createRequest("/", { method: "OPTIONS" }), "root");
expect(false).toBe(true);
} catch (data) {
expect(isErrorWithStatus(data)).toBe(true);
expect(data.status).toBe(405);
expect(data.message).toBe('Invalid request method "OPTIONS"');
}
});

try {
await queryRoute(
createSubmitRequest("/", { method: "OPTIONS" }),
"root"
);
expect(false).toBe(true);
} catch (data) {
expect(data instanceof Response).toBe(true);
expect(data.status).toBe(405);
expect(data.statusText).toBe("Method Not Allowed");
expect(data.headers.get("X-Remix-Router-Error")).toBe("yes");
expect(await data.text()).toBe("");
}
/* eslint-enable jest/no-conditional-expect */
});
});
Expand Down
2 changes: 2 additions & 0 deletions packages/router/index.ts
Expand Up @@ -29,10 +29,12 @@ export type {
export {
AbortedDeferredError,
ErrorResponse,
ErrorWithStatus,
defer,
generatePath,
getToPathname,
invariant,
isErrorWithStatus,
isRouteErrorResponse,
joinPaths,
json,
Expand Down