Skip to content

chore(ssr): show the user land ssr error stack#12714

Open
sun0day wants to merge 5 commits into
vitejs:mainfrom
sun0day:chore/ssrLoaderErrorFormat
Open

chore(ssr): show the user land ssr error stack#12714
sun0day wants to merge 5 commits into
vitejs:mainfrom
sun0day:chore/ssrLoaderErrorFormat

Conversation

@sun0day

@sun0day sun0day commented Apr 3, 2023

Copy link
Copy Markdown
Member

Description

refs #12631 (comment)

Before PR:

image

After PR:

image

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@bolt-new-by-stackblitz

Copy link
Copy Markdown

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@sun0day sun0day changed the title chore(ssr): show the user land srr error stack chore(ssr): show the user land ssr error stack Apr 3, 2023
@sun0day

sun0day commented Apr 3, 2023

Copy link
Copy Markdown
Member Author

@brillout cc

@brillout

brillout commented Apr 3, 2023

Copy link
Copy Markdown
Contributor

Love it 😍.

I wonder whether the errors can be further bundled? In principle, I believe there are only two errors: one while executing /pages/index/index.page.tsx and another one while executing /renderer/_default.page.server.tsx.

(FYI: if vite-plugin-ssr encounters an error, then vite-plugin-ssr tries to render the error page which may encounter another error. That's why there can be up to two errors for a single HTTP request.)

@sun0day

sun0day commented Apr 4, 2023

Copy link
Copy Markdown
Member Author

Love it 😍.

I wonder whether the errors can be further bundled? In principle, I believe there are only two errors: one while executing /pages/index/index.page.tsx and another one while executing /renderer/_default.page.server.tsx.

(FYI: if vite-plugin-ssr encounters an error, then vite-plugin-ssr tries to render the error page which may encounter another error. That's why there can be up to two errors for a single HTTP request.)

Yeah. We could minimize the error output, you can check the updated description. I think it's good enough for now

@brillout

brillout commented Apr 4, 2023

Copy link
Copy Markdown
Contributor

Nice, very pretty.

So there is no way to get rid of the redundant first error?

Little nitpick: how about the following?

Error while executing /pages/index/index.page.tsx on the server-side:

@sun0day

sun0day commented Apr 4, 2023

Copy link
Copy Markdown
Member Author

So there is no way to get rid of the redundant first error?

That would be hard since the error comes from different catch

Ah I found it's possible. See https://user-images.githubusercontent.com/102238922/229852488-34f993ab-c090-47ca-86bf-163beb51a173.png

Error while executing /pages/index/index.page.tsx on the server-side:

Not sure about this. Not much difference between this and Error when evaluating SSR module /pages/index/index.page.tsx in my opinion.

Any idea? @bluwy @patak-dev

@bluwy

bluwy commented Apr 8, 2023

Copy link
Copy Markdown
Member

Yeah I think we should keep the current error messages style, the file path at the end makes it easier to follow.

@bluwy bluwy added p2-nice-to-have Not breaking anything but nice to have (priority) feat: ssr labels Apr 8, 2023
Comment thread packages/vite/src/node/ssr/ssrModuleLoader.ts Outdated
Comment thread packages/vite/src/node/ssr/ssrModuleLoader.ts Outdated
@brillout

brillout commented Apr 8, 2023

Copy link
Copy Markdown
Contributor

I'd argue that "server-side" is clearer than "SSR". Not only doesn't everyone know what "SSR" means but also "SSR module" is actually a misnomer. SSR = Server-side Rendering while a "SSR module" may have nothing do to with SSR (e.g. pre-rendered SSG apps).

As for "executing" VS "evaluating": I think for people who aren't fluent in computer/English language, "executing" is more familiar than "evaluation". ("Evaluating" is quite a computer science term.)

@bluwy bluwy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A question below. If you have an example project to test the errors you can share, that'll be great too 🙏 Trying to understand how the flow of this works.


// only log the root url's error
if (
errorDepsStack.filter((dep) => !dep.startsWith('virtual:'))[0] === url

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason this is filtered by virtual:? I'm not sure if it's right to handle virtual: specifically.

Also, to confirm, is logging the root check helps deduplicate errors?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

  1. Ah I may make a mistake here since virtual module should be in the error stack
  2. It will prevent the same error from outputting multiple times in a root-leaf path.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I reviewed the virtual: filtering logic again, the error would not propagate to the virtual module(not sure why), so we have to filter it. @bluwy

e.g urlStack is

['virtual:vite-plugin-ssr:importUserCode:server',
  '/renderer/_error.page.tsx',
  '\x00react/jsx-dev-runtime']

The error-catched url would never be virtual:vite-plugin-ssr:importUserCode:server'

@sun0day

sun0day commented Apr 12, 2023

Copy link
Copy Markdown
Member Author

Check this repo @bluwy

@brillout

Copy link
Copy Markdown
Contributor

Check this repo @bluwy

Relevant to the repo: #12714 (comment).

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

Labels

feat: ssr p2-nice-to-have Not breaking anything but nice to have (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants