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

Conversation

hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Nov 20, 2023

Closes: #8065

I added DevServerHooks.fixStacktrace to pass ViteDevServer.ssrFixStacktrace to server runtime, then server runtime uses it to extend default behavior of handleError during vite dev.

Testing Strategy

I verified the local build on the reproduction repo hi-ogawa/remix-vite-stacktrace-repro#1
I also added playwright test to check error stacktrace line/column in dev error boundary.

Copy link

changeset-bot bot commented Nov 20, 2023

🦋 Changeset detected

Latest commit: 09dcf0a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/dev Patch
@remix-run/server-runtime Patch
@remix-run/cloudflare Patch
@remix-run/deno Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/architect Patch
@remix-run/express Patch
@remix-run/serve Patch
@remix-run/testing Patch
create-remix Patch
remix Patch
@remix-run/css-bundle Patch
@remix-run/eslint-config Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@hi-ogawa hi-ogawa changed the title fix(vite): apply ssrFixStacktrace to server error fix(remix-dev/vite, server-runtime): apply ssrFixStacktrace to server error Nov 22, 2023
@hi-ogawa hi-ogawa marked this pull request as ready for review November 23, 2023 04:52
@markdalgleish markdalgleish self-assigned this Nov 23, 2023
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 😅

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.

let devServerHooks = getDevServerHooks();
if (devServerHooks && error instanceof Error) {
devServerHooks.fixStacktrace(error);
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.

"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 👍

Copy link
Member

@markdalgleish markdalgleish left a comment

Choose a reason for hiding this comment

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

Nice one! Thanks for the PR 👍

@markdalgleish markdalgleish changed the title fix(remix-dev/vite, server-runtime): apply ssrFixStacktrace to server error fix(remix-dev/vite, server-runtime): pass Vite server errors to vite.ssrFixStacktrace Nov 24, 2023
@markdalgleish markdalgleish merged commit dda449a into remix-run:dev Nov 24, 2023
9 checks passed
@hi-ogawa hi-ogawa deleted the vite-ssrFixStacktrace branch November 24, 2023 00:44
Copy link
Contributor

github-actions bot commented Dec 5, 2023

🤖 Hello there,

We just published version 2.4.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 2.4.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants