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

feat(reactivity): new effectScope API #2195

Merged
merged 8 commits into from
Jul 7, 2021
Merged

Conversation

antfu
Copy link
Member

@antfu antfu commented Sep 21, 2020

RFC: vuejs/rfcs#212

Progress

  • more feedback & RFC process
  • effectScope implemented
  • test cases
  • refactor @vue/runtime-core to reuse the logic
  • onCleanup & returns forwarding updates

@antfu
Copy link
Member Author

antfu commented Oct 15, 2020

@knightly-bot build this

@knightly-bot
Copy link

Nightly Build

🌒 Knightly build enabled, release every night at 00:00 UTC (skip if no change)

📦 npm package

@antfu antfu changed the title feat(reactivity): new effectScope API [DRAFT] feat(reactivity): new effectScope API Oct 21, 2020
@antfu antfu marked this pull request as ready for review October 21, 2020 04:42
@antfu antfu force-pushed the feat/effect-scope branch from c10761d to 51ee6dc Compare October 21, 2020 16:19
@basvanmeurs
Copy link
Contributor

It would be a nice feature to be able to reuse a previously created effect scope.

Use case: global utilities such as localizations might want to use reactivity as well. For example, translations data may be made reactive, and lookups may be cached using a computed internally. Those computeds may be lazily created.

These global utilities may be called from component instances. This may cause newly created effects by the localization module to end up in the instance scope. This may cause problems when that instance's scope is stopped: computeds and watchers become broken.

What is needed for global modules is a way to:

  • create a new effect scope and obtain a reference to it
  • 'escape' the effect scope stack
  • re-activate the effect scope later when new effects need to be created

The only thing still missing is the ability to re-activate an effect scope by reference. Do you agree on the use case?

@antfu antfu marked this pull request as draft November 24, 2020 08:44
@antfu

This comment has been minimized.

@antfu antfu force-pushed the feat/effect-scope branch from 51ee6dc to 7954219 Compare November 26, 2020 08:56
@@ -0,0 +1,3 @@
export function warn(msg: string, ...args: any[]) {
console.warn(`[Vue warn] ${msg}`, ...args)
Copy link
Member Author

@antfu antfu Nov 27, 2020

Choose a reason for hiding this comment

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

Since warn for Vue is presented in runtime-core binding with the component model, is it ok to introduce this util in reactivity?

@antfu antfu force-pushed the feat/effect-scope branch from 3798190 to acbf976 Compare December 3, 2020 03:44
@antfu antfu marked this pull request as ready for review December 3, 2020 03:57
@antfu
Copy link
Member Author

antfu commented Dec 3, 2020

@basvanmeurs Sorry I missed your comment. The ability to "re-activate" a scope is indeed interesting. But as the effect is not re-activatable with the current implementation, I'd suggest you open a RFC to add the ability to resume a stopped effect. After that, we can easily adapt it to the effectScope. What do you think?

@basvanmeurs
Copy link
Contributor

basvanmeurs commented Dec 3, 2020

@basvanmeurs Sorry I missed your comment. The ability to "re-activate" a scope is indeed interesting. But as the effect is not re-activatable with the current implementation, I'd suggest you open a RFC to add the ability to resume a stopped effect. After that, we can easily adapt it to the effectScope. What do you think?

Thanks for your response @antfu

But I don't mean to re-activate the scope, but rather reuse a still active scope. Like this:

class MyModule {
  private cachedNumber?: ComputedRef<number> = undefined;
  private scope = effectScope(() => {});
  public getCachedNumber() {
    if (!this.cachedNumber) {
      effectScope(() => {
        this.cachedNumber = computed(() => 1);
      }, { scope });
    }
    return this.cachedNumber;
  }
  destroy() {
     this.scope.stop();
  }
}
export const myModule = new MyModule();

This is handy for external modules that wish to utilize reactivity.

Imagine you'd call myModule.getCachedNumber() from a component setup for the first time. The computed will be incorrectly part of the component's scope and be stopped when that component is destroyed. When another component would fetch getCachedNumber it would get a stopped computed.

Another tricky situation would be async stuff:

doSomething() {
  effectScope(() => {
    fetch("./something.txt").then(data => this.myComputed = computed(() => data));
  });
}

This would allow creating effects from async responses. I don't see how we could do that the current API within the correct scope. We need to enforce a scope in that case:

doSomething() {
  const scope = effectScope(() => {});
  fetch("./something.txt").then(data => effectScope(() => {this.myComputed = computed(() => data}, scope));
}

Or, possibly, with an adjusted api:

doSomething() {
  const scope = effectScope();
  fetch("./something.txt").then(data => scope.exec(() => { this.myComputed = computed(() => data) }));
}

@yyx990803 yyx990803 added the 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. label Dec 4, 2020
@basvanmeurs
Copy link
Contributor

basvanmeurs commented Jul 1, 2021

@antfu I'd like to use this PR to solve memory leaks in my project. Could you give me write permissions on this branch so that I can resolve the conflicts for 3.2?

I would like to run some performance tests as well. One thing is that I'd prefer to use the class-based approach in line with the class-based ReactiveEffect. Another possible problem is that the stop-function is invoked with many different possible types. This might affect performance.

@basvanmeurs
Copy link
Contributor

basvanmeurs commented Jul 1, 2021

@antfu Could we please expose the currently active scope as well? This would allow us to reuse the active scope in setup asyncs:

    const scope = getActiveEffectScope();
    fetch(...).then(() => {
      extendScope(scope, () => {
        effect(() => (doubled = counter.num * 2))
      })
    });

The current implementation would require us to create an unnecessary sub-scope because we need to reference something in extendScope.

@basvanmeurs
Copy link
Contributor

basvanmeurs commented Jul 1, 2021

Besides, I think that a warning should be thrown when an effect is created outside of any effectScope:

setup() {
  fetch(...).then(() => {
    watchEffect(..)
  });
}

Notice, due to the promise, that watchEffect is executed outside of the component's effectScope (in fact: outside of any effectScope). This always causes a memory leak, as watchEffect will be referenced by its dependencies. It will always stay responsive to dependencies as well, affecting performance after a lot of these components were removed.

We should protect developers against making this mistake. So if no activeEffectScope exists when invoking the effect function, a warning should be added (in __DEV__ mode). Something in the lines of effect() is called outside of a scope, causing a memory leak: use extendScope()

@antfu antfu force-pushed the feat/effect-scope branch from 4a06595 to b657587 Compare July 1, 2021 20:45
@antfu
Copy link
Member Author

antfu commented Jul 1, 2021

@basvanmeurs I have resolved the conflict.

getActiveEffectScope

I have add the new API getEffectScope to the RFC. Will implement it later

warning should be thrown when an effect is created outside of any effectScope

There are may cases we could have effect outside of an effectScope, for example, module level computed and watch. If we expect them to be global and active though the entire lifetime, there is no actual "memory leak". I would think is this the caveat of the reactivity system and ppl should be able to take care of it themself.

@basvanmeurs
Copy link
Contributor

Thanks for the getEffectScope addition @antfu.

I can see your point about the global effect scope 👍

Thanks for resolving but @yyx990803 has added your PR to the roadmap for 3.2 (https://github.com/vuejs/vue-next/projects/4). I think that you need to rebase your PR to branch 3.2? The 3.2 branch contains more changes to the reactivity module than master, including the conversion of the function-style effect to the ReactiveEffect class, which certainly has conflicts with your PR.

@0x009922
Copy link

0x009922 commented Jul 3, 2021

It would be great feature! When developing various reactive functionality, every now and then I came across the fact that scopes are tied strictly to the components' lifecycle, which is rather a special case, and this RFC seems to be an integral part of the entire vue reactivity API.

Btw, 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
})

@antfu, is it implementable?

@antfu
Copy link
Member Author

antfu commented Jul 3, 2021

@0x009922 please go to the RFC for design discussions. For the return forwarding, here are the original discussions vuejs/rfcs#212 (comment)

@yyx990803 yyx990803 changed the base branch from master to 3.2 July 5, 2021 18:22
@yyx990803 yyx990803 force-pushed the feat/effect-scope branch from d55b47b to 91cac5d Compare July 5, 2021 20:32
@yyx990803
Copy link
Member

Just rebased on 3.2 and split effectScope specific logic into a separate file.

I may do some more fine-tuning for memory and bundle size optimizatons.

@yyx990803
Copy link
Member

yyx990803 commented Jul 5, 2021

Did some major refactoring in dabfeaf and 464bd59:

  • Use class-based implementation for memory-efficiency (also removed the options object, detached is now simply a constructor argument)
    • effectScope() -> new EffectScope(detached?)
    • scope.extend -> scope.run()
  • The scope itself now exposes run and stop methods.
    • extendScope removed. Use scope.run(fn).
    • Change to stop reverted. Use scope.stop().
  • Removed storing/merging return values on scope. scope.run(() => val) simply returns val. This also allows us to remove a lot of unnecessary concepts/APIs:
    • isEffectScope
    • isEffectScopeReturns
    • EffectScopeReturns
    • EffectScopeReturnsWrapper
  • onScopeStopped -> onScopeDispose
  • The onStop argument passed to the executed scope function has been removed - this avoids allocating an inline function even when the inner function doesn't expect any arguments. Instead, just use onScopeDispose directly.

So there are now only 3 new APIs added:

  • EffectScope (class)
  • getCurrentScope
  • onScopeDispose

The main reason for the simplification is because this is a low-level API that should prioritize efficiency and simplicity. The effect scope API is used in @vue/runtime-core's critical path so it cannot be tree-shaken, so size is also an important consideration. With the refactoring the size increase on top of current 3.2 branch is also reduced from 0.15kb to 0.02kb.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. version: minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants