Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.3k
feat(vue): experimental scope dispose#5382
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
|
Hebilicious commentedOct 3, 2025
@Andarist I've been testing this in production, it appears to be an improvement. Would you mind taking a look please? |
| setup() { | ||
| const actor=useActorRef(machine, {}, (nextState)=> { | ||
| state.value=nextState.value; | ||
| nextTick(()=> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not familiar with Vue at all so bear with me. I don't understand why this is usingnextTick. Shouldn't the update be batched with whatever changes are in flight instead of waiting for the current DOM updates to flush and then scheduling a new update by updatingstate.value here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ah, I guess u have touched on it in the PR's description. So a different question - wouldnt it be better to skip the write to a =n uninitializedstate instead of wrapping this update withnextTick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would never write vue code like that in the first place withnextTick either, but it kinda works, even though it is hacky... I don't think we should encourage this pattern tbh, imo lifecycle hooks are the right pattern to initalize a value, as documented in the PR description.
But I wanted to hear your opinion instead of straight-up replacing the text.
| letsub:Subscription|undefined; | ||
| if(observerOrListener){ | ||
| sub=actorRef.subscribe(toObserver(observerOrListener)); | ||
| } | ||
| actorRef.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
how this is meant to work with SSR? when thesub would get disposed (and when the actor would get stopped!) in that environment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You're right, onMounted and onUnmounted do not get called in SSR. This should work and respect current behaviour.
if(getCurrentScope()){if(observerOrListener){sub=actorRef.subscribe(toObserver(observerOrListener));}actorRef.start();}
What do you think ?
| actorRef.stop(); | ||
| sub?.unsubscribe(); | ||
| }); | ||
| if(getCurrentScope()){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldnt we require a scope to be there? if it's going to be missing then cleanup code wouldnt ever run but setup would and its obtained resources would live indefinitely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Might as well wrap everything inif(getCurrentScope()) since this should not run on SSR.
Andarist commentedOct 28, 2025
What would be the advantage of moving to this API? how that would affect supported range of Vue versions? |
Hebilicious commentedOct 30, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hi@Andarist, One of the reasons I've started doing this in all my vue + xstate projects is because very often, you want to Consider the following real world example, where I'm constructing data to pass it to tanstack table : constdata=computed(()=>operations.map(({ id, operation})=>{conststate=useSelector(operation,(state)=>state)const{ text, kind}=statusFromState(state.value.value,state.value.context.direction)return{id,operation,status:state.value.value,statusText:text,statusKind:kind,source:state.value.context.source,destination:state.value.context.destination,amount:state.value.context.amount,hash:state.value.context.transaction?.hash??"",date:state.value.context.transaction?.date.toISOString()??newDate().toISOString()}})) Current useSelector implementation causes a bunch of "issues" with vue because component lifecycles don't exist outside a component setup function. Regarding version support, I believe it got introduced in 3.2https://blog.vuejs.org/posts/vue-3-2, so this is reasonable (4y old +), but would require a bump to 3.2 min (https://github.com/statelyai/xstate/blob/main/packages/xstate-vue/package.json) |
Uh oh!
There was an error while loading.Please reload this page.
This is a draft PR that attempts to move away from component life-cycle hooks in composables toeffect scopes.
All tests are passing, with one caveat :
Now
nextTickneeds to be used when passing a callback touseActorRefthat access state that is not initialized.I feel like this is fine because this is kind of an odd pattern anyways.
Using
onMountedis more idiomatic to vue imo, and maybe we should change the test to :(Users should not be using useActorRef directly in most cases anyways, so its fine if the test doesn't use shallowRef)