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

Reactivity's effectScope API #212

Merged
merged 23 commits into from
Jul 14, 2021
Merged

Reactivity's effectScope API #212

merged 23 commits into from
Jul 14, 2021

Conversation

antfu
Copy link
Member

@antfu antfu commented Sep 20, 2020

@CyberAP
Copy link
Contributor

CyberAP commented Sep 20, 2020

Not strictly against this proposal, but I don't completely understand the motivation point. I have several questions if you don't mind:

  1. What are the real word applications of this? How frequently does this happen\might happen?
  2. What kind of problems do require that style of authoring reactivity?
  3. What are the challenges of collecting stop handlers? (See example below)
  4. How wrapping this in a callback scope would solve this? (maybe creating a special higher-order watcher could work in that case)
  5. How is it going to affect Composition API? In particular how it would interplay with the existing setup cleanup behaviour?

Also the code could be authored in a more simplified way, which might help with the issue:

const disposables = []

const counter = ref(0)
const doubled = computed(() => counter.value * 2)

disposables.push(() => stop(doubled.effect))

disposables.push(
  watchEffect(() => {
    console.log(`counter: ${counter.value}`
  }))
)

disposables.push(watch(doubled, () => console.log(double.value)))

disposables.forEach(f => f())
disposables = []

@antfu
Copy link
Member Author

antfu commented Sep 20, 2020

  1. If you like to make your own a framework. For example, @vue/lit, it does not handle effects in the component instance lifecycles, which will cause mem leakage if you try to mount and unmount the sub-component multiple times. In order to solve this, you will need to implement recordInstanceBoundEffect for your framework, which is not an easy & simple task from the outside. And also they will lose the ability to be reused interchangeably with Vue's ecosystem.

  2. Clean up side-effects, preventing mem leak.

  3. I don't see your code being "simplified". A huge amount of disposables.push( will make the logic hard to read. And if you take the following example, things will become more complex.

import { useMyLogic1, useMyLogic2 } from './utils'
import { useLogicFromLibrary } from 'xxx-compsoable'

const disposables = []

// each your custom logic should do the collecting in order to be disposed by the user side
const { state1, disposables: disposables1 } = useMyLogic1()

disposables.push(...disposables1)

const { state2, disposables: disposables2 } = useMyLogic2()

disposables.push(...disposables2)

// it does not collect the disposables, how do you dispose it? (note this is ok in setup)
const state3 = useLogicFromLibrary()
  1. Similar to what setup does.

  2. This actually more like "extracting the setup functionality, generalized and abstract into @vue/reactivity", and the goal is to be reused in setup() as well.

Thanks for the feedback, and hope I answered your concerns.

@CyberAP
Copy link
Contributor

CyberAP commented Sep 20, 2020

And also they will lose the ability to be reused interchangeably with Vue's ecosystem.

Why is that important?

This actually more like "extracting the setup functionality, generalized and abstract into @vue/reactivity", and the goal is to be reused in setup() as well.

This resembles a solution to the specific component-based framework problems, but outside of these frameworks what would be the case when you need to do a lot of effect creation and disposal? Cases when we need to do a lot of cleanup are of the most interest to me.

@antfu
Copy link
Member Author

antfu commented Sep 20, 2020

Why is that important?

It's not that important, but why not if you can? Like I might be able to set up an alias and make VueUse work for @vue/lit, so I don't need to reinvent another set of utils that doing the same thing for it or yet another framework. They could even contribute back to Vue's ecosystem.

what would be the case when you need to do a lot of effect creation and disposal?

Even outside of Vue's instance this could be a problem. Maybe I would like to build a state management system my own, I might need some side-effects and to be cleaned afterward. I need to collect them.

More widely, I can use it in VSCode extension (I already did), I need to watch on config changes, editor changes, file system changes... I would also need to clean them up when I no longer need some. I might use it in a server to do something like HMR which is based on local file changes. When some file gets deleted, I also need to dispose the fs watchers.

There are so many possibilities for this great reactivity system. And we just got it for only a while now.

@jods4
Copy link

jods4 commented Sep 20, 2020

I think this is nice because the concept is already in Vue but not cleanly abstracted.
The fact that Vue itself needs this shows that there is a use case.
It's already in the framework, so instead of making a special case just for components, why not extract a reusable API?

It won't get much use when @vue/reactivity is used in UI though, as Vue already provides the very useful Component == reactive scope, but it could be handy if you're doing something that is not based on components. And it's impossible to do from outside, without heavy hacks.

In particular how it would interplay with the existing setup cleanup behaviour?

If this is accepted, the existing cleanup should 100% be based on this new API.

This would give us an easy way to perform something that is really hacky today: escape component lifetime (some users have already asked how to do this in issues during the beta).
Opening a nested scope would escape the component scope:

setup() {
  // Stopped when component is unmounted
  watch(...)
  // This one won't be stopped automatically
  effectScope(() => {
    watch(...)
  })
}

I'm wondering if this API could have more advanced uses by exposing the captured effects, maybe through an effects property on the returned stop function?
To be honest I have no use case for that right now. I was thinking along the lines being able to force a refresh of everything in a scope, or maybe for tooling / debuggers, or detecting that a piece of code is effect-free (leading to static optimizations?).

@privatenumber
Copy link

Great proposal! I love to see support for making @vue/reactivity work more conveniently as a standalone package, especially since being used independently of Vue is advertised as a pretty big feature of Vue 3 (eg. brought up multiple times in the Vue 3 announcement keynote).

@jods4

If @vue/runtime-dom setup is refactored to use effectScope behind the scenes, I think any new effectScope instantiated inside setup will also be cleaned up as per the Nested Scopes spec.

I think this can still be worked around though, simply by instantiating effectScope outside of setup, but if users really want to create a scope unbound to the component lifespan, I think they'll always find a way.

@jods4
Copy link

jods4 commented Sep 20, 2020

If @vue/runtime-dom setup is refactored to use effectScope behind the scenes, I think any new effectScope instantiated inside setup will also be cleaned up as per the Nested Scopes spec.

That's a good point.
I think Components don't work like that today -- they only stop effects that were created inside them, not their children (which get stopped by unmounting the whole tree, but that's not the same mechanism).

This is incentive to change the spec slightly so that nested effectScopes are not stopped by the outer one, so that Components can be implemented on top of that spec.
If there is a good use-case for the opposite behavior, then maybe this could be controlled by an optional flag passed to effectScope (if there is no use case today, such a flag can always be added later).

IMHO not implementing components on top of this spec feels bad because:

  • it's exactly the kind of use-case for that spec, it'd be weird that the framework itself can't make use of it;
  • it's duplicated code for practically the same thing;
  • then the interaction between component scope and effectScope must be specified clearly, leading to added complexity, possibly weird edge cases and unnecessary coupling between reactivity and components.

think this can still be worked around though, simply by instantiating effectScope outside of setup, but if users really want to create a scope unbound to the component lifespan, I think they'll always find a way.

It's been asked already and I know of at least two ways you can achieve this today... but both are hacky. It'd be a nice bonus if this spec allowed that in a clean way through a documented API.

@antfu
Copy link
Member Author

antfu commented Sep 20, 2020

Update: effectScope now returns the scope instance instead of the stop handler. Providing more flexibility and make the stop api more general.

const scope = effectScope(() => {
  const doubled = computed(() => counter.value * 2)

  watch(doubled, () => console.log(double.value))

  watchEffect(() => console.log('Count: ', double.value))
})

// to dispose all effects in the scope
stop(scope)

@unbyte
Copy link

unbyte commented Sep 21, 2020

I really like its concept
however I have doubts about it 🙋‍♂️

  1. in most cases computed values would be used as meaningful variables, and closures complicate its use. and it will lead to some bug codes if computed values can be exported from the closure, since stopping effectScope breaks reactivity of exported computed values that may still be used without notification.

  2. if stopping a parent scope also stops all child scopes, how to create a standalone scope inside a scope (it's useful)? and if can by certain api, how to stop this standalone scope simply since it's in a nested closure?

@CyberAP
Copy link
Contributor

CyberAP commented Sep 21, 2020

What do you think of attaching effects to the scope after scope creation? That might come handy for effects that are initiated asynchronously.

const scope = effectScope()

setTimeout(() => {
  scope(() => {
    watch(...)
  })
})

Or attach by pushing:

const scope = effectScope()

setTimeout(() => {
  scope.push(watch(...))
})

If the scope has been destroyed the effect should not be created.

@antfu
Copy link
Member Author

antfu commented Sep 21, 2020

@unbyte

and it will lead to some bug codes if computed values can be exported from the closure

Can't image it, can you provide some example of why to use computed outside the scope? Need some context so maybe I can think about how to change the design to fit the equipment.

how to create a standalone scope inside a scope

Nested scope should be collected by its parent scope, but can still be disposed explicitly

const scope1 = effectScope(() => {
  

  const scope2 = effectScope(() => {
    watchEffect(/* ... */)
  ))
  
  stop(scope2)
})

@CyberAP

What do you think of attaching effects to the scope after scope creation?

With the last change, effectScope() returns the scope instance instead of the stop handler. You can do that by

const scope = effectScope()

setTimeout(() => {
  scope.effects.push(watch(...))
})

@unbyte
Copy link

unbyte commented Sep 21, 2020

@antfu

Can't image it, can you provide some example of why to use computed outside the scope? Need some context so maybe I can think about how to change the design to fit the equipment.

I may get you... so you mean all related logic codes are inside the scope right?

Nested scope should be collected by its parent scope, but can still be disposed explicitly

😂 my mean is how to make the child scope have a longer life than the parent scope and to be stopped outside, for example vuejs/core#1532

@mathe42
Copy link

mathe42 commented Sep 21, 2020

😂 my mean is how to make the child scope have a longer life than the parent scope and to be stopped outside, for example vuejs/vue-next#1532

Maybe we can have a namespace so

const scope1 = effectScope('namespaceA', () => {
  
  const scope2 = effectScope('namespaceB', () => {
    watchEffect(/* ... */)
  ))

  const scope3 = effectScope('namespaceA', () => {
    watchEffect(/* ... */)
  ))
})
stop(scope1)

So scope2 is still running but scope3 is not. Also the namespace could be optional and when omited it is useing a default namespace.

@unbyte
Copy link

unbyte commented Sep 21, 2020

@mathe42 so in this way users should stop a scope by its name string. it looks easy but not so good for coding (personnally).

and the dependency of each layer in the nested scopes is not always one-way, which may cause some problem when clean up effects under certain namespace.

maybe I have the wrong idea to solve this problem

@antfu
Copy link
Member Author

antfu commented Sep 21, 2020

vuejs/core#1532 seems like a reasonable use case, that would be great if we can solve it along with this RFC. I am thinking if we could add an option to it and make it not be collected by its parent. Something like:

let scope2

const scope1 = effectScope(() => {
  watchEffect(/* ... */)

  scope2 = effectScope(() => {
    watchEffect(/* ... */)
  ), { escaped: true })
})

stop(scope1)
// scope2 will still be alive until you dispose it

@antfu
Copy link
Member Author

antfu commented Sep 21, 2020

Update:

  • "escaped" options added
  • Able to extend the existing scope

A draft PR is filed with the feature itself implemented. vuejs/core#2195

@unbyte
Copy link

unbyte commented Sep 22, 2020

@antfu if reuse it in runtime-core,effectScope may have to return some values for rendering

maybe it can be like this

const scope = effectScope(()=>{
    return {
        // some value
    }
})
scope.exported // visit exported value

child scopes' return value are exported by parent too. it should be a flat object.

but I still don't have a good way to solve the naming conflict

@antfu
Copy link
Member Author

antfu commented Sep 22, 2020

Actually, I have already made the refactor to runtime-core in the draft PR and all tests are passing ;)

effectScope may have to return some values for rendering

Did think about that once, can't find a clean design for it though. And TBH, I don't think this feature is a must-have, you can do it like

let result
const scope = effectScope(()=>{
    result = {
        // some value
    }
})

console.log(result)

@0x009922
Copy link

0x009922 commented Jul 3, 2021

(forwarding vuejs/core#2195 (comment))

I don't like "Forward returning values" section:

const scope = effectScope(() => {
  const { x } = useMouse()
  const doubled = computed(() => x.value * 2)

  return { x, doubled }
})

const { x, doubled } = scope

console.log(doubled.value)

Here I do not like the fact that (1) what is returned by the function is affected, since it can generate some conflicts and it is generally not clear why it should be affected, and (2) there is a restriction on returning exactly the object, and it is also not clear why is it.

It seems to me that it would be more elegant and simpler to return a tuple with a return value and a scope, or only a scope if the function is void (returns undefined):

const [{ x, doubled }, scope] = effectScope(() => {
  const { x } = useMouse()
  const doubled = computed(() => x.value * 2)

  return { x, doubled }
})

const scope2 = effectScope(() => {
  const counter = useInterval(1_000)
  watchEffect(() => console.log(counter.value))
  // no return
})

It seems to me that this is simple both in implementation and in typing.

@yyx990803
Copy link
Member

So I did a full review of the API design while rebasing vuejs/core#2195. Since this is going to be a very low-level API, the API shape needs to be designed with implementation constraints in mind. I decided to greatly simplify the proposed API in order to obtain smaller size increase and better memory efficiency. details

I will be revising this PR to reflect the updates.

@CyberAP
Copy link
Contributor

CyberAP commented Jul 5, 2021

What do you think of a more fine-grained control over the scope effect execution?

For example if we want to run effects on some changes (or while some flag is set), but to stop on others?

const foo = ref(0)
const scope = new EffectScope(() => {
  effect(() => { console.log(foo.value) })
})
foo.value = 1 // runs as usual
scope.pause()
animateValue(foo) // doesn't run during animated change
  .then(() => { scope.resume() }) 

If we'd like to replicate current scope.stop() we could destroy the scope:

scope.destroy() // can not be resumed nor paused

@yyx990803
Copy link
Member

@CyberAP that's not a designed capability of the effect scope, at least in this RFC. The way it is currently designed is only meant for collecting effects so they can be stopped together.

@yyx990803
Copy link
Member

I revised the RFC to match the latest implementation - it is now much simpler:

const scope = new EffectScope()

const returnValue = scope.run(() => { /* ... */ }

scope.stop()

Please read the full rendered version.

In particular, I expanded the usage example to illustrate a new pattern this would unlock: Shared Composables.

@antfu
Copy link
Member Author

antfu commented Jul 6, 2021

Thanks for the reviewing, @yyx990803 ! Love the updates and now it's much more concise and elegant!

Just an opinionated thought, would it better align with Vue's other APIs if we use a function to construct the instance instead of new? The reason I am thinking this is that in Vue 3 most of the APIs are functions that starts with lowercase letters, createApp(), defineComponent(), ref() while the capitalized exports are more commonly referring to type interfaces, App, Component, Ref, ComputedRef. Maybe we could use effectScope(), createEffectScope() or createScope() for this API?

If we do, I think we could have the module implemented like this:

class EffectScope {
  /* ... */
}

export function effectScope() {
  return new EffectScope()
}

export type { EffectScope }

While we could always do it in the user land, just raising this for discussions.

@yyx990803
Copy link
Member

That makes sense - added the effectScope factory function for API consistency.

@yyx990803 yyx990803 added the final comments This RFC is in final comments period label Jul 6, 2021
@yyx990803
Copy link
Member

This RFC is now in final comments stage. An RFC in final comments stage means that:

The core team has reviewed the feedback and reached consensus about the general direction of the RFC and believe that this RFC is a worthwhile addition to the framework.
Final comments stage does not mean the RFC's design details are final - we may still tweak the details as we implement it and discover new technical insights or constraints. It may even be further adjusted based on user feedback after it lands in an alpha/beta release.
If no major objections with solid supporting arguments have been presented after a week, the RFC will be merged and become an active RFC.

@AlexandreBonaventure
Copy link

Wow this is truely amazing!
Funny enough we've built and use in production our own implementation of almost the same API to build shareable compositions for a year already.
https://github.com/AlexandreBonaventure/vue-scoped-composition

This pattern is so powerful and I'm really stoked that Vue is going to provide built-ins for that.
The RFC API is elegant and consistent, great job!

@udany
Copy link

udany commented Jul 10, 2021

Looks great, I'd been thinking about this tangentially but hadn't quite put my finger on it before reading this proposal :D

If you guys allow me a last minute suggestion, we could make the syntax shorter for the most common use cases by accepting a function on the constructor/factory that would get automatically run, like this:

const scope = effectScope (() => { /* ... */ });

Instead of:

const scope = effectScope ();
scope.run(() => { /* ... */ })

@SeregPie
Copy link

@udany
Function run returns the value the callback returns and the constructor has an optional detached argument.

@yyx990803 yyx990803 merged commit b20f29a into master Jul 14, 2021
@yyx990803 yyx990803 deleted the reactivity-effect-scope branch July 14, 2021 15:32
@daniele-orlando
Copy link

daniele-orlando commented Jul 27, 2021

@antfu @yyx990803

About the signature of effectScope, my comment is that it could be improved

  1. from clearness
  2. and future proof

points of view, adopting an options object as first argument, instead of a plain boolean.

Current Signature

effectScope(detached = false): EffectScope

effectScope(true) // true what?

Declarative and Future Proof

effectScope(EffectScopeOptions): EffectScope

effectScope({ detached: true })

interface EffectScopeOptions {
    detached?: boolean
}

@antfu
Copy link
Member Author

antfu commented Jul 27, 2021

@daniele-orlando Thanks for bringing this up. Just discussed with the team, here is a short summary:

The main reason we use a plain boolean for this argument is performance concern. Giving the fact that this API is an advanced and low-level API, trading a little bit of readability and extendibility allows it to save a few object allocation and restructuring cost (which could be significant on higher level when using the low-level api hundreds of times). You can find the similar reason for the optimization against the effect api vuejs/core#4001

@daniele-orlando
Copy link

Hi @antfu and thanks for the answer.

I see the rational behind the choice.
From what I understand, effectScope is serving two domains, the public APIs domain and the internal APIs domain.

From the public APIs domain point of view, passing an object should not be a performance problem, given that it is called only during the setup only once or few times per component and only in advanced cases. Similar use case of watch() which accepts an options object as third argument.

From the internal APIs domain point of view, passing an object can be a performance penalty and should be avoided.

Given that effectScope() is already a wrapper around new EffectScope(), would not be the case to provide a user friendly Public effectScope(options) API that maps to the Internal and optimised effectScope(detached: boolean)?

Just wondering, I'have no knowledge of the Vue internals, so I really don't if it could be too cumbersome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final comments This RFC is in final comments period
Projects
None yet
Development

Successfully merging this pull request may close these issues.