-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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: do not fixStacktrace #7995
Conversation
33ed1f1
to
0e938f0
Compare
Wouldn't it be better to remove all of these stack trace fixing api's and just fix sourcemap support for dev mode ssr? #3288 |
0e938f0
to
800aef6
Compare
Sourcemaps work for the browser, but the stack trace fixing APIs are helpful when printing an exception to the console. If we did want to go in that direction though this PR would still be a helpful step |
Right, nodejs also supports sourcemaps specifically for the purpose of fixing the stack trace: https://nodejs.org/dist/latest-v18.x/docs/api/cli.html#--enable-source-maps I believe the vite dev server should be updated to always enable source map support in node since it can be enabled programmatically and it'd make this transparent to the end user. |
Ah, interesting. Thanks for sharing! I wasn't aware of that. I think this PR would be a good step to take given that. It stops fixing stack traces so that people can either use that flag or manually fix the stack trace using the Vite-provided APIs. Then people can experiment and make sure that the flag works just as well for all the various frameworks. I have some concern that having to specify a flag on the CLI is less user-friendly, but with this PR each framework would have the option to try which ever method they expect would work best and we can get feedback from the ecosystem community |
Right, I agree. However it can be enabled manually and I was suggesting vite would just do that by default in dev mode (https://docs.nodejs.org/api/process.html#processsetsourcemapsenabledval) |
@benmccann we talked about the change with the team. Let's disable |
800aef6
to
f8e8286
Compare
Thanks @patak-dev for the review and discussing with the team. I've updated this PR accordingly |
Description
Closes #7528
Users should call
ssrFixStacktrace
themselves when catching an exception. This avoids issues with that method possibly being called multiple timesThis fixes the playground examples, which all currently have broken exception handling because they all call
vite && vite.ssrFixStacktrace(e)
. That is invalid because Vite currently calls that method internally and so it ends up being called twice which breaks the stack tracesAdditional context
For Vite 3.0 since this is a breaking change
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).