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(remix-dev/vite, server-runtime): pass Vite server errors to vite.ssrFixStacktrace #8066

Merged
merged 12 commits into from
Nov 24, 2023
6 changes: 6 additions & 0 deletions .changeset/mean-knives-beam.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@remix-run/dev": patch
"@remix-run/server-runtime": patch
---

Pass request handler errors to `vite.ssrFixStacktrace` in Vite dev to ensure stack traces correctly map to the original source code
56 changes: 56 additions & 0 deletions integration/vite-dev-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,39 @@ test.describe("Vite dev", () => {
</>
}
`,
"app/routes/error-stacktrace.tsx": js`
import type { LoaderFunction, MetaFunction } from "@remix-run/node";
import { Link, useLocation } from "@remix-run/react";

export const loader: LoaderFunction = ({ request }) => {
if (request.url.includes("crash-loader")) {
throw new Error("crash-loader");
}
return null;
};

export default function TestRoute() {
const location = useLocation();

if (import.meta.env.SSR && location.search.includes("crash-server-render")) {
throw new Error("crash-server-render");
}

return (
<div>
<ul>
{["crash-loader", "crash-server-render"].map(
(v) => (
<li key={v}>
<Link to={"/?" + v}>{v}</Link>
</li>
)
)}
</ul>
</div>
);
}
`,
"app/routes/known-route-exports.tsx": js`
import { useMatches } from "@remix-run/react";

Expand Down Expand Up @@ -385,6 +418,29 @@ test.describe("Vite dev", () => {
expect(pageErrors).toEqual([]);
});

test("request errors map to original source code", async ({ page }) => {
let pageErrors: unknown[] = [];
page.on("pageerror", (error) => pageErrors.push(error));

await page.goto(
`http://localhost:${devPort}/error-stacktrace?crash-server-render`
);
await expect(page.locator("main")).toContainText(
"Error: crash-server-render"
);
await expect(page.locator("main")).toContainText(
"error-stacktrace.tsx:16:11"
Copy link
Member

Choose a reason for hiding this comment

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

This is really nice 👍

);

await page.goto(
`http://localhost:${devPort}/error-stacktrace?crash-loader`
);
await expect(page.locator("main")).toContainText("Error: crash-loader");
await expect(page.locator("main")).toContainText(
"error-stacktrace.tsx:7:11"
);
});

test("handle known route exports with HMR", async ({ page }) => {
let pageErrors: unknown[] = [];
page.on("pageerror", (error) => pageErrors.push(error));
Expand Down
11 changes: 9 additions & 2 deletions packages/remix-dev/vite/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -669,9 +669,9 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => {
setTimeout(showUnstableWarning, 50);
});

// Give the request handler access to the critical CSS in dev to avoid a
// flash of unstyled content since Vite injects CSS file contents via JS
setDevServerHooks({
// Give the request handler access to the critical CSS in dev to avoid a
// flash of unstyled content since Vite injects CSS file contents via JS
getCriticalCss: async (build, url) => {
invariant(cachedPluginConfig);
return getStylesForUrl(
Expand All @@ -682,6 +682,13 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => {
url
);
},
// If an error is caught within the request handler, let Vite fix the
// stack trace so it maps back to the actual source code
processRequestError: (error) => {
Copy link
Member

Choose a reason for hiding this comment

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

Heads up that I've renamed this hook from fixStacktrace to processRequestError. We want to make sure that all dev server hooks are named in a Vite-agnostic way. As part of this decoupling from Vite, I've also made all of the hooks optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't usually have a strong opinion (and good sense) on naming things, so please feel free and don't hesitate to tweak anything I named.

decoupling from Vite

That sounds like a huge plan!
I'll keep in mind that when doing things in the future.

Copy link
Member

Choose a reason for hiding this comment

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

We don't see any need for these specific hooks to be used outside of Vite. For now it's mainly just to avoid the weirdness of having references to Vite in @remix-run/server-runtime 😅

Choose a reason for hiding this comment

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

I am getting errors such as:

Error: `line` must be greater than 0 (lines start at line 1)

and I am wondering if related to these changes.

if (error instanceof Error) {
vite.ssrFixStacktrace(error);
}
},
});

// We cache the pluginConfig here to make sure we're only invalidating virtual modules when necessary.
Expand Down
3 changes: 2 additions & 1 deletion packages/remix-server-runtime/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,11 @@ export function logDevReady(build: ServerBuild) {
}

type DevServerHooks = {
getCriticalCss: (
getCriticalCss?: (
build: ServerBuild,
pathname: string
) => Promise<string | undefined>;
processRequestError?: (error: unknown) => void;
};

const globalDevServerHooksKey = "__remix_devServerHooks";
Expand Down
9 changes: 7 additions & 2 deletions packages/remix-server-runtime/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,17 @@ export const createRequestHandler: CreateRequestHandlerFunction = (
let url = new URL(request.url);

let matches = matchServerRoutes(routes, url.pathname);
let handleError = (error: unknown) =>
let handleError = (error: unknown) => {
if (mode === ServerMode.Development) {
Copy link
Member

@markdalgleish markdalgleish Nov 24, 2023

Choose a reason for hiding this comment

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

I added this check because we want to make sure these hooks are never called outside of dev, especially since the hooks are coming from a global that can be modified by anyone.

getDevServerHooks()?.processRequestError?.(error);
}

errorHandler(error, {
context: loadContext,
params: matches && matches.length > 0 ? matches[0].params : {},
request,
});
};

let response: Response;
if (url.searchParams.has("_data")) {
Expand Down Expand Up @@ -137,7 +142,7 @@ export const createRequestHandler: CreateRequestHandlerFunction = (
} else {
let criticalCss =
mode === ServerMode.Development
? await getDevServerHooks()?.getCriticalCss(_build, url.pathname)
? await getDevServerHooks()?.getCriticalCss?.(_build, url.pathname)
: undefined;

response = await handleDocumentRequestRR(
Expand Down
Loading