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

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sun0day
Copy link
Member

@sun0day sun0day commented Apr 3, 2023

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.

@stackblitz
Copy link

stackblitz bot commented Apr 3, 2023

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
Copy link
Member Author

sun0day commented Apr 3, 2023

@brillout cc

@brillout
Copy link
Contributor

brillout commented Apr 3, 2023

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
Copy link
Member Author

sun0day commented Apr 4, 2023

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
Copy link
Contributor

brillout commented Apr 4, 2023

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
Copy link
Member Author

sun0day commented Apr 4, 2023

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
Copy link
Member

bluwy commented Apr 8, 2023

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
@brillout
Copy link
Contributor

brillout commented Apr 8, 2023

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.)

Copy link
Member

@bluwy bluwy left a comment

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
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
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
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
Copy link
Member Author

sun0day commented Apr 12, 2023

Check this repo @bluwy

@brillout
Copy link
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