Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Remove instanceof Response checks in favor of isResponse #4782

Merged
merged 3 commits into from
Dec 6, 2022
Merged

Remove instanceof Response checks in favor of isResponse #4782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
981acc8
Remove instanceof Response checks in favor of isResponse
brophdawg11 Dec 6, 2022
42e2232
Add changeset
brophdawg11 Dec 6, 2022
f2ac118
Bump to @remix-run/router 1.0.5-pre.1
brophdawg11 Dec 6, 2022
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/rare-bikes-report.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/server-runtime": patch
---

Remove `instanceof Response` checks in favor of isResponse"
30 changes: 29 additions & 1 deletion integration/loader-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,27 +69,48 @@ test.describe("loader in an app", () => {
let appFixture: AppFixture;

let HOME_PAGE_TEXT = "hello world";
let REDIRECT_TARGET_TEXT = "redirect target";
let FETCH_TARGET_TEXT = "fetch target";

test.beforeAll(async () => {
appFixture = await createAppFixture(
await createFixture({
files: {
"app/root.jsx": js`
import { Outlet } from '@remix-run/react'

export default function Root() {
return (
<html>
<body>
${HOME_PAGE_TEXT}
<Outlet />
</body>
</html>
);
}
`,
"app/routes/redirect.jsx": js`
import { redirect } from "@remix-run/node";
export const loader = () => redirect("/");
export const loader = () => redirect("/redirect-target");
export default () => <div>Yo</div>
`,
"app/routes/redirect-target.jsx": js`
export default () => <div>${REDIRECT_TARGET_TEXT}</div>
`,
"app/routes/fetch.jsx": js`
export function loader({ request }) {
return fetch(new URL(request.url).origin + '/fetch-target');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the case we're fixing here - returning a raw fetch Response wasn't matching our instanceof Response check

}
`,

"app/routes/fetch-target.jsx": js`
import { json } from "@remix-run/node";

export function loader() {
return json({ message: "${FETCH_TARGET_TEXT}" })
}
`,
},
})
);
Expand All @@ -103,5 +124,12 @@ test.describe("loader in an app", () => {
let app = new PlaywrightFixture(appFixture, page);
await app.goto("/redirect");
expect(await app.getHtml()).toMatch(HOME_PAGE_TEXT);
expect(await app.getHtml()).toMatch(REDIRECT_TARGET_TEXT);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strengthened the existing redirect tests a bit to ensure it renders the right content at the redirected location.

});

test("handles raw fetch responses", async ({ page }) => {
let app = new PlaywrightFixture(appFixture, page);
let res = await app.goto(`/fetch`);
expect((await res.json()).message).toBe(FETCH_TARGET_TEXT);
});
});
2 changes: 1 addition & 1 deletion packages/remix-server-runtime/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
"typings": "dist/index.d.ts",
"module": "dist/esm/index.js",
"dependencies": {
"@remix-run/router": "1.0.5-pre.0",
"@remix-run/router": "1.0.5-pre.1",
"@types/cookie": "^0.4.0",
"@web3-storage/multipart-parser": "^1.0.0",
"cookie": "^0.4.1",
Expand Down
10 changes: 5 additions & 5 deletions packages/remix-server-runtime/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import type { RouteMatch } from "./routeMatching";
import { matchServerRoutes } from "./routeMatching";
import type { ServerRoute, ServerRouteManifest } from "./routes";
import { createStaticHandlerDataRoutes, createRoutes } from "./routes";
import { json, isRedirectResponse } from "./responses";
import { json, isRedirectResponse, isResponse } from "./responses";
import { createServerHandoffString } from "./serverHandoff";

export type RequestHandler = (
Expand Down Expand Up @@ -123,7 +123,7 @@ async function handleDataRequestRR(

return response;
} catch (error) {
if (error instanceof Response) {
if (isResponse(error)) {
error.headers.set("X-Remix-Catch", "yes");
return error;
}
Expand Down Expand Up @@ -217,7 +217,7 @@ async function handleDocumentRequestRR(
return new Response(null, { status: 500 });
}

if (context instanceof Response) {
if (isResponse(context)) {
return context;
}

Expand Down Expand Up @@ -373,12 +373,12 @@ async function handleResourceRequestRR(
let response = await staticHandler.queryRoute(request, routeId);
// callRouteLoader/callRouteAction always return responses
invariant(
response instanceof Response,
isResponse(response),
"Expected a Response to be returned from queryRoute"
);
return response;
} catch (error) {
if (error instanceof Response) {
if (isResponse(error)) {
// Note: Not functionally required but ensures that our response headers
// match identically to what Remix returns
error.headers.set("X-Remix-Catch", "yes");
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2069,10 +2069,10 @@
"@changesets/types" "^5.0.0"
dotenv "^8.1.0"

"@remix-run/router@1.0.5-pre.0":
version "1.0.5-pre.0"
resolved "https://registry.npmjs.org/@remix-run/router/-/router-1.0.5-pre.0.tgz#d5327cc606b444b9721dd0856ad18760e8d75d6d"
integrity sha512-7UWQ6HcuNHbKeCiOKD94WDz+FAeeJVzxVi+GNeLczzEdJtqKAvYMiXqJ/IF84T8479+bUrEm/zp7hr+itZs8Zg==
"@remix-run/router@1.0.5-pre.1":
version "1.0.5-pre.1"
resolved "https://registry.npmjs.org/@remix-run/router/-/router-1.0.5-pre.1.tgz#dd0939d1631ebd56faa2bc69141e94a96887c7e5"
integrity sha512-dDfVwp0ta99+7vMKMDb5eh1Pc9CWZ5/Q+679YojyHyrZqSWJmlZyCP7prs6UAaXaGg5MveP1pCav/YFCINlcFw==

"@remix-run/web-blob@^3.0.3", "@remix-run/web-blob@^3.0.4":
version "3.0.4"
Expand Down