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): reset current instance if setting up options component errors (fix #7733) #7743

Merged
merged 2 commits into from
May 19, 2023

Conversation

DrPhil
Copy link
Contributor

@DrPhil DrPhil commented Feb 17, 2023

Warning: I'm just a monkey with a wrench - I don't know what I am doing.
Looking at the PR of #6184 I think this might be something close-ish to the right fix. It at least fixes the tests I hallucinated together. 😄

Maybe @danielroe would be interested in reviewing this fix too?

close #7733

unsetCurrentInstance()
try {
applyOptions(instance)
resetTracking()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious whether the resetTracking() call should be inside the try or the finally.

Are you anticipating resetTracking() throwing an error?

If applyOptions() throws an error, would we not want to reset the tracking too, so that we don't leave that stack in a paused state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ook-ook! 🙈🔧 I'm not sure what even a trackStack is, and much less what state it is supposed to be after an error in applyOptions. 😄

I was confused about where to put that line: it does sound like one ought to reset the tracking after one has paused it. All the tests (all the tests that I could get to run) worked for both versions. My final thought process was to make a minimal patch to fix the problem I had. Before this patch we didn't reset tracking after errors in applyOptions, after this patch we still don't reset tracking after errors in applyOptions.

I've checked "allow edits by maintainers", so maintainers can edit as they like. If you are not a maintainer, feel free to try to find an argument (or better, a bug reproduction) and I'll change it. 😃

@sxzz
Copy link
Member

sxzz commented Feb 23, 2023

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Feb 23, 2023

📝 Ran ecosystem CI: Open

suite result
nuxt ✅ success
pinia ✅ success
router ✅ success
test-utils ✅ success
vite-plugin-vue ✅ success
vitepress ✅ success
vue-macros ✅ success
vueuse ✅ success

@DrPhil
Copy link
Contributor Author

DrPhil commented Apr 11, 2023

Just checking that it's not me that we are waiting for. Is there something else I should do to get this merged? There's this one unresolved comment, but I'm not sure what the right resolution is for it. If there's anything else I can do, please let me know.

try {
applyOptions(instance)
} finally {
resetTracking()
Copy link
Member

Choose a reason for hiding this comment

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

small change - moved resetTracking() into finally block too.

try {
await render(
createApp({
errorCaptured() {
Copy link
Member

Choose a reason for hiding this comment

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

errorCaptured only captures errors from descendants, so a separate child component is needed.

@yyx990803 yyx990803 merged commit 020851e into vuejs:main May 19, 2023
@DrPhil DrPhil deleted the fix-7733 branch May 22, 2023 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Current instance is not unset after error in data() on SSR
6 participants