Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # 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