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(ssr): skip rewriting stack trace if it's already rewritten (fixes #11037) #11070

Merged
merged 4 commits into from
Dec 8, 2022

Conversation

sapphi-red
Copy link
Member

Description

See #11037 (comment) for the reason of this bug.

This PR will change vite.ssrFixStacktrace to work like: After rewriting stack trace, add a property to indicate that. If the error included that property, skip rewriting stack trace.

fixes #11037

Additional context

I considered the following alternatives.

Changing the error cache

By running this script, you can see the error instance is same between both imports.

try {
  await import('./bar.mjs') /* content: `throw new Error('bar')` */
} catch (e) {
  console.log(e.foo) // undefined
  e.foo = 'modify'
  console.log(e.foo) // 'modify'
}

try {
  await import('./bar.mjs')
} catch (e) {
  console.log(e.foo) // 'modify'
}

Vite's ssrLoadModule's semantics aligns with Node.js's import by this cache.
So I think we cannot change this part.

Making ssrFixStacktrace non-destructive

The usage of vite.ssrFixStacktrace will be like:

const rewroteError = vite.ssrFixStacktrace(e)
throw rewroteError

This will be a breaking change. We could introduce a new method, but that will still require users to change the code to avoid #11037 happening.
Also I didn't find a good way to clone Error instance.


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

@sapphi-red sapphi-red added p3-minor-bug An edge case that only affects very specific usage (priority) feat: ssr labels Nov 25, 2022
await server.ssrLoadModule('/fixtures/modules/has-error.js')
} catch (e) {
expect(e[s]).toBe(true)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was passing before this PR but I added to ensure this won't break in future.

@benmccann
Copy link
Collaborator

We added an option to avoid rewriting the stack trace awhile back, which is how we've dealt with this: #7046

@sapphi-red
Copy link
Member Author

The repro of #11037 uses that option.

app.use('*', async (req, res) => {
  try {
    const render = (await vite.ssrLoadModule('/src/entry-server.js')).render // fixStacktrace is `false` by default

    const url = req.originalUrl.replace(base, '')
    const rendered = await render(url, ssrManifest)

    res.status(200).set({ 'Content-Type': 'text/html' }).end(rendered)
  } catch (e) {
    vite?.ssrFixStacktrace(e)
    console.log(e.stack)
    res.status(500).end(e.stack)
  }
})

Does SvelteKit do it like this to avoid #11037?

const rewroteErrors = new WeakSet()
app.use('*', async (req, res) => {
  try {
    const render = (await vite.ssrLoadModule('/src/entry-server.js')).render // fixStacktrace is `false` by default

    const url = req.originalUrl.replace(base, '')
    const rendered = await render(url, ssrManifest)

    res.status(200).set({ 'Content-Type': 'text/html' }).end(rendered)
  } catch (e) {
    if (!rewroteErrors.has(e)) {
      vite?.ssrFixStacktrace(e)
      rewroteErrors.add(e)
    }
    console.log(e.stack)
    res.status(500).end(e.stack)
  }
})

bluwy
bluwy previously approved these changes Nov 29, 2022
@bluwy
Copy link
Member

bluwy commented Nov 29, 2022

Does SvelteKit do it like this to avoid #11037?

I don't think so, but I also haven't heard of this happening in SvelteKit. But I think it's good that we fix this generically though so that it always works.

@patak-dev patak-dev enabled auto-merge (squash) December 8, 2022 15:21
@patak-dev patak-dev merged commit feb8ce0 into vitejs:main Dec 8, 2022
@sapphi-red sapphi-red deleted the fix/ssr-rewrite-stacktrace-once branch December 8, 2022 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

line must be greater than 0 error happens with ssrFixStacktrace when same error happens the second time
4 participants