fix(nuxt): don't overwrite scope in runWithContext if scope already exists

[daniluk4000]

🔗 Linked issue

Resolves #25029

📚 Description

This issue can cause significant problems for anybody using root watch on CSR with defineNuxtComponent.

Turns out the issue is runWithContext callback for setup. I don't think that removing that completely is needed since on SSR it's a good feature if you're using asyncContext.

So I've decided to simply remove this callback on client since there context is standalone.

Fun fact: getCurrentInstance is not undefined there, so it could also possibly be upstream vue bug, but I'm not sure about it.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[daniluk4000]

Video proof

2024-04-28.010229.1.mp4

[danielroe]

runWithContext, in conjunction with the async transform provided by unctx, is meant to be what keeps watch and Nuxt instance preserved.

It may be we need to fix the bug in another way. Can you add a regression test in this PR?

[daniluk4000]

runWithContext, in conjunction with the async transform provided by unctx, is meant to be what keeps watch and Nuxt instance preserved.

It may be we need to fix the bug in another way. Can you add a regression test in this PR?

Okie, I'll try to do that tomorrow. I still believe it could be Vue bug.

Also I don't see if this could break something since unctx probably does nothing on CSR in this case, isn't it?

[danielroe]

Currently the Nuxt app context is set on client side, but this is not necessarily guaranteed in future.

nuxtApp.runWithContext also ensures that we use vueApp.runWithContext to preserve Vue app context after awaits...

[daniluk4000]

Currently the Nuxt app context is set on client side, but this is not necessarily guaranteed in future.

nuxtApp.runWithContext also ensures that we use vueApp.runWithContext to preserve Vue app context after awaits...

Maybe vueApp.runWithContext is where bug actually is.

I'll still add tests for this issue. Should I make separate PR for it or keep it there?

[danielroe]

Should I make separate PR for it or keep it there?

As you like! Depending on the solution we find, we can still treat this PR as the source of truth, and update it ...

[daniluk4000]

@danielroe it's done. Test passes with my changes and doesn't pass without them.

[daniluk4000]

It seems like units have bad time due to ui templates... I can't run any tests after merge

[daniluk4000]

I looked more into the code and I agree that my solution would not be good.

However, I've found the issue. It's nuxtApp._scope.run(fn). If I leave callWithNuxt only without this, everything is working just fine.

I'm not an expert in Vue reactivity, but I can guess that this is the global scope that isn't disposed, and that's exactly the reason why watchers aren't cleared - they're not supposed to be.

[daniluk4000]

[daniluk4000]

I've pushed new implementation.

1. Added optional second argument for runWithContext: scope. Defaults to nuxtApp._scope
   • Should we make second argument an object for future params grow support?
   • Should we exclude that from types (with expect-error in defineNuxtComponent) instead to make this fully internal?
2. Only apply scope in runWithContext when there's no other scope available, use given scope as default
3. Transfer getCurrentScope from defineNuxtComponent to runWithContext just in case something goes wrong

[daniluk4000]

Should be good now. Would you need me to port this to Bridge when approved?

Also: if some await will appear before runWithContext in defineNuxtComponent this solution will break (without second argument).

I don't know how much is that risk I'll let you decide that :)