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

Jump to bottom

fix(remix-dev/vite, server-runtime): pass Vite server errors to vite.ssrFixStacktrace #8066

Merged
merged 12 commits into from
Nov 24, 2023
Merged

fix(remix-dev/vite, server-runtime): pass Vite server errors to vite.ssrFixStacktrace #8066

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1396b16
fix(vite): apply `ssrFixStacktrace` to server error
hi-ogawa Nov 20, 2023
5988807
Merge branch 'dev' into vite-ssrFixStacktrace
hi-ogawa Nov 20, 2023
25e865c
test: add test
hi-ogawa Nov 21, 2023
9b7b08f
chore: changeset
hi-ogawa Nov 21, 2023
4eb7194
Merge branch 'dev' into vite-ssrFixStacktrace
hi-ogawa Nov 22, 2023
22eae1a
Merge branch 'dev' into vite-ssrFixStacktrace
hi-ogawa Nov 23, 2023
cc9d5f9
refactor: add DevServerHooks.fixStackTrace
hi-ogawa Nov 23, 2023
6708f3e
chore: rename
hi-ogawa Nov 23, 2023
16022e4
Merge branch 'dev' into vite-ssrFixStacktrace
markdalgleish Nov 23, 2023
7b0dd6a
update changeset
markdalgleish Nov 24, 2023
6ded334
rename hook to processRequestError
markdalgleish Nov 24, 2023
09dcf0a
tweak test name
markdalgleish Nov 24, 2023
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
Prev Previous commit
Next Next commit
rename hook to processRequestError
  • Loading branch information
@markdalgleish
markdalgleish committed Nov 24, 2023
commit 6ded334a2d1931c4da24b3ee044826b7d6e04f45
12 changes: 9 additions & 3 deletions packages/remix-dev/vite/plugin.ts
View file
Open in desktop
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,7 +682,13 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => {
url
);
},
fixStacktrace: vite.ssrFixStacktrace,
// 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 😅

Copy link

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
4 changes: 2 additions & 2 deletions packages/remix-server-runtime/dev.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ export function logDevReady(build: ServerBuild) {
}

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

const globalDevServerHooksKey = "__remix_devServerHooks";
Expand Down
8 changes: 4 additions & 4 deletions packages/remix-server-runtime/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,10 @@ export const createRequestHandler: CreateRequestHandlerFunction = (

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

@markdalgleish markdalgleish Nov 24, 2023 •
edited
Loading

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 : {},
Expand Down Expand Up @@ -142,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