fix(ssr): reset current instance if setting up options component errors (fix #7733) #7743
Conversation
| unsetCurrentInstance() | ||
| try { | ||
| applyOptions(instance) | ||
| resetTracking() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. 😃
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
|
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() |
There was a problem hiding this comment.
small change - moved resetTracking() into finally block too.
| try { | ||
| await render( | ||
| createApp({ | ||
| errorCaptured() { |
There was a problem hiding this comment.
errorCaptured only captures errors from descendants, so a separate child component is needed.
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