fix: ensure ssrRewriteStacktrace doesn't throw

[paoloricciuti]

This is a weird edge case so feel free to just close this PR if it doesn't make sense.

Basically right now there's a weird bug in webcontainers where any error during ssr makes the dev server crash. The reason it happens is because ssrRewriteStackTrace it's called twice (for some reason it doesn't happen locally).

This PR fixes this behavior by wrapping ssrRewriteStackTrace in a try/catch and returning the stack as is if it throws.

Again i understand this is a small enough edge case but i tought that given that sometimes ssrRewriteStackTracecan throw would still be a good addition.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

[changeset-bot]

🦋 Changeset detected

Latest commit: afaa26d

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

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	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

[benmccann]

This looks like a good change to me, but I'd like to get some more details. How do we reproduce this issue? Any idea why ssrRewriteStackTrace is called twice? (I'm assuming that shouldn't happen). Can you share a stack trace from a crash?

[paoloricciuti]

This looks like a good change to me, but I'd like to get some more details. How do we reproduce this issue? Any idea why ssrRewriteStackTrace is called twice? (I'm assuming that shouldn't happen). Can you share a stack trace from a crash?

Unfortunately it's a super weird issue that I was not able to reproduce consistently...i only seen it happen in webcontainers but I while I was implementing an hacky fix for sveltelab (SvelteLab/SvelteLab#278) I discovered that locally it doesn't happen and it only happens on the deployed version on vercel.

I was able to reproduce in stackblitz too tho.

[benmccann]

How often does it happen? Can you reproduce it somewhat consistently or is it very rare? I wonder if we should log that an error was encountered rather than silently ignoring it? On the one hand, I don't want to annoy users, but on the other, I don't want to just ignore that something bad is happening and cover up the underlying bug rather than fixing it

@gtm-nayan pointed out the other day that Vite added a check to avoid rewriting stack traces twice in vitejs/vite@feb8ce0. As a result, we may not need fix_stack_trace anymore and could just rely on Vite as we added our code to avoid having stack traces written twice before Vite did

[paoloricciuti]

How often does it happen? Can you reproduce it somewhat consistently or is it very rare? I wonder if we should log that an error was encountered rather than silently ignoring it? On the one hand, I don't want to annoy users, but on the other, I don't want to just ignore that something bad is happening and cover up the underlying bug rather than fixing it

@gtm-nayan pointed out the other day that Vite added a check to avoid rewriting stack traces twice in vitejs/vite@feb8ce0. As a result, we may not need fix_stack_trace anymore and could just rely on Vite as we added our code to avoid having stack traces written twice before Vite did

I've also opened an issue on webcontainers with the reproduction. But basically either go on sveltelab or stackblitz and throw an error in the load function.

Here's the issue on webcontainers ( stackblitz/webcontainer-core#1155 )

[paoloricciuti]

Maybe the best course of action would be still wrapping the call in a try catch given that it could throw but add the error to the returned stack in the catch?

[benmccann]

@paoloricciuti does the error still happen in the latest release since #10769 was merged? If so, this PR will need a rebase

[paoloricciuti]

@paoloricciuti does the error still happen in the latest release since #10769 was merged? If so, this PR will need a rebase

Uh I missed the linked PR...I have to check, I'll do it in a moment

[paoloricciuti]

It seems to work fine on stackblitz ( I can't test on sveltelab because I was temporarily patching sveltekit with this solution to make it work)

[gtm-nayan]

You don't need this change now, right?

[paoloricciuti]

You don't need this change now, right?

No it seems to have been fixed by #10769