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: add pause/resume methods for render function #9206

Closed
wants to merge 15 commits into from

Conversation

Alfred-Skyblue
Copy link
Member

@Alfred-Skyblue Alfred-Skyblue commented Sep 13, 2023

fixed #5386

To address the issue of KeepAlive repeatedly executing the effect in the deactivate state, I have added pause and resume methods to ReactiveEffect for pausing and resuming the execution of the effect. Currently, it is used in the KeepAlive component. When the component is in the deactivated state, it pauses the execution of the effect and keeps track of whether it was called during the pause. When the component is in the activated state, it resumes the execution of the effect and immediately executes it once if the run function was called during the pause.

@github-actions
Copy link

github-actions bot commented Sep 13, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 86.8 kB (+349 B) 33 kB (+112 B) 29.8 kB (+77 B)
vue.global.prod.js 133 kB (+349 B) 49.8 kB (+129 B) 44.6 kB (+85 B)

Usages

Name Size Gzip Brotli
createApp 48.3 kB (+283 B) 19 kB (+89 B) 17.3 kB (+73 B)
createSSRApp 51.5 kB (+283 B) 20.3 kB (+83 B) 18.5 kB (+65 B)
defineCustomElement 50.7 kB (+283 B) 19.8 kB (+83 B) 18 kB (+82 B)
overall 61.7 kB (+349 B) 23.9 kB (+127 B) 21.7 kB (+147 B)

@Alfred-Skyblue Alfred-Skyblue marked this pull request as draft September 13, 2023 15:16
@Alfred-Skyblue Alfred-Skyblue marked this pull request as ready for review September 13, 2023 15:23
@Alfred-Skyblue
Copy link
Member Author

Before:
update1

Now:

update2

@tafelnl
Copy link

tafelnl commented Sep 19, 2023

This looks and sounds promising. Could you maybe elaborate on to what extend this is the same as what @BenceSzalai explained in #5386 (comment). I think this is what he calls 'level 5' right?

Also you mentioned in #5386 (comment) that it's more difficult to achieve for v-show. Does that mean that this PR doesn't solve it (yet) for components that are using the v-show attribute and were hidden at the time of navigating away?

@Alfred-Skyblue
Copy link
Member Author

The reason why v-show cannot be processed is because v-show is the data obtained during the update process. If we pause the execution of the effect, the element will never be re-displayed.

@posva posva added the need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. label Sep 19, 2023
@Alfred-Skyblue
Copy link
Member Author

Alfred-Skyblue commented Sep 20, 2023

For computed and watch, we need access to the fn's return value when dealing with oldValue. During the pause period, the run function returns undefined, which can result in incorrect values for watch and computed. Therefore, the pause and resume functionality should be limited to the render function. I've introduced a derived class called RenderEffect to manage the execution of the render function.

@Alfred-Skyblue Alfred-Skyblue changed the title feat: add pause/resume function to ReactiveEffect feat: add pause and resume methods for render function Sep 20, 2023
@Alfred-Skyblue Alfred-Skyblue changed the title feat: add pause and resume methods for render function feat: add pause/resume methods for render function Sep 20, 2023
@Alfred-Skyblue
Copy link
Member Author

Regarding the v-show directive, when it's bound to a component, it forces the child component to update along with the parent component's updates. However, v-show does not affect the rendering structure inside the component; it is solely used to control the display/hidden state of the component. Currently, this behavior causes the child component to be updated regardless of whether the component is in a visible state, which is not necessary under the v-show directive.

Related code:

if (nextVNode.dirs || nextVNode.transition) {

@Alfred-Skyblue
Copy link
Member Author

Can someone take a look at this PR?

@Alfred-Skyblue
Copy link
Member Author

Alfred-Skyblue commented Sep 26, 2023

This feature allows me to use IntersectionObserver to control updates, which can significantly improve performance in the project.

@netlify
Copy link

netlify bot commented Oct 17, 2023

Deploy Preview for vue-sfc-playground failed.

Name Link
🔨 Latest commit a5446a3
🔍 Latest deploy log https://app.netlify.com/sites/vue-sfc-playground/deploys/652e9650c2d9d80008d8e07d

@Alfred-Skyblue
Copy link
Member Author

@posva Sorry for bothering you, could you take a look at this PR?

@@ -127,6 +127,8 @@ const KeepAliveImpl: ComponentOptions = {

sharedContext.activate = (vnode, container, anchor, isSVG, optimized) => {
const instance = vnode.component!
// on activation, resume the effect of the component instance and immediately execute the call during the pause process
instance.effect.resume(true)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could have a prop for KeepAlive to toggle this behavior (and default disabling it to keep existing behaviour)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we can add a prop to preserve the original behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a lazy prop to control whether it should pause in the deactivated state.

} else {
return super.run()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Love the compatibility of pausable effect. Since this is not bound to the runtime/dom, I wonder if this could be generally available in ReactiveEffect directly.

/cc @yyx990803 WDYT? Or do you think it deserves a dedicated RFC?

Copy link
Member Author

@Alfred-Skyblue Alfred-Skyblue Nov 14, 2023

Choose a reason for hiding this comment

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

As the addition of pause and resume methods in ReactiveEffect has significant implications for the existing codebase, I have initiated an RFC to discuss this feature.

vuejs/rfcs#599

/cc @yyx990803 @antfu

@antfu antfu added 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. need discussion labels Nov 10, 2023
@Alfred-Skyblue
Copy link
Member Author

I have added the pause/resume functionality to the ReactiveEffect class. As the ReactiveEffect class has a wide impact, I have opened a new PR #9651 to implement this feature.

@Alfred-Skyblue Alfred-Skyblue changed the base branch from main to minor November 29, 2023 04:36
@Alfred-Skyblue Alfred-Skyblue marked this pull request as draft December 28, 2023 14:04
@ferferga
Copy link
Contributor

ferferga commented Jan 8, 2024

@Alfred-Skyblue Besides Keep-Alive, I guess this is something that could also be applicable here?: vuejs/rfcs#501

@Alfred-Skyblue
Copy link
Member Author

@ferferga The functionality has temporarily shifted to be implemented in #9651. I noticed your mention in the RFC that state changes in internal components during the execution of a Transition animation would disrupt the animation state. Regarding the solution you proposed:

Stop dependency/effect tracking of the child component recursively.

Currently, we have implemented detachment for the scope of component instances in the EffectScope. Considering performance reasons, recursively pausing might lead to performance issues due to excessive depth. As of now, the solution I can think of is to manually call scope.pause during the animation execution and manually call scope.resume after the animation is completed. Of course, this approach may seem cumbersome.

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. need discussion need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. version: minor
Projects
Status: Rejected
Development

Successfully merging this pull request may close these issues.

Kept alive components still update (renders, watchers) while being deactivated
6 participants