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): add pause/resume methods to ReactiveEffect #9651

Merged
merged 46 commits into from
Aug 2, 2024

Conversation

Alfred-Skyblue
Copy link
Member

@Alfred-Skyblue Alfred-Skyblue commented Nov 21, 2023

RFC: vuejs/rfcs#599

In this PR, a pause/resume method has been added to the effect to control whether the effect can be executed. When the effect is in a paused state and the run method is called, it will save the call once, and it can be immediately executed after calling the resume method. This is used in certain situations to improve performance by reducing unnecessary execution of effects, such as pausing in the deactivated state of the keep-alive component, or implementing lazy updates in conjunction with IntersectionObserver.

In #9206, derived classes were added for extension, aimed at controlling the pause/resume of the render function. Following @antfu's suggestion, the pause and resume methods were added to the ReactiveEffect class. As the ReactiveEffect class has a widespread impact, a new PR has been opened.

Copy link

github-actions bot commented Nov 21, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 96.1 kB (+6.03 kB) 36.6 kB (+2.06 kB) 33 kB (+1.83 kB)
vue.global.prod.js 154 kB (+7 kB) 56.4 kB (+2.35 kB) 50.1 kB (+2.04 kB)

Usages

Name Size Gzip Brotli
createApp 53.8 kB (+4.19 kB) 20.9 kB (+1.38 kB) 19 kB (+1.21 kB)
createSSRApp 57.8 kB (+4.57 kB) 22.5 kB (+1.52 kB) 20.4 kB (+1.34 kB)
defineCustomElement 56.1 kB (+4.19 kB) 21.6 kB (+1.41 kB) 19.7 kB (+1.26 kB)
overall 67.4 kB (+4.25 kB) 25.9 kB (+1.39 kB) 23.5 kB (+1.25 kB)

Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

I think in general this is a feature nice to have. And we will find it quite useful in VueUse to handle deactivate/resume effects

@Disservin
Copy link
Contributor

Not strictly related to the pr, but why are some private members starting with an _ while others dont? Is there some set style guide? I'd expect one or the other not both.

@Alfred-Skyblue Alfred-Skyblue changed the title feat(reactivity): add pause/resume methods to effect feat(reactivity): add pause/resume methods to ReactiveEffect Nov 21, 2023
@Alfred-Skyblue Alfred-Skyblue changed the base branch from main to minor November 29, 2023 04:33
@Alfred-Skyblue Alfred-Skyblue marked this pull request as ready for review December 5, 2023 04:32
@ferferga
Copy link
Contributor

ferferga commented Jan 9, 2024

@Alfred-Skyblue (Follow-up of this discussion)

If there's not a better way to pass the "paused" state down (with inject/provide for example) and must be done with recursivity, instead of doing it from the top to the bottom as you suggested, what if it's done from the bottom to the top? I think it might be more efficient, since if I understood everything correctly, your proposal is O(n^d) and mine might be O(n) (in the best case).

Imagine a frozen component (I think it's a good naming! More at the bottom...) that has 3 children and each children has another 10 components. With your suggestion, from the parent to the bottom, you'd need to first tell the 3 children that they're frozen, and these 3 need to do the same for their 10 children. That is 33 ops.

However, what if the component that detects an update ask the parent chain if it's frozen before executing? You're assuming all children will need to trigger effects at some point, but they might not. In our previous example, only one of the inner childs might have an effect triggered: that child just needs to "ask" it's parent, which will "ask" the frozen component and freeze itself (so further requests from inner children will stop there). This should be more efficient (and we're not even taking into account the performance improvements of not altering the DOM while transitions are taking place!).

I'm not so into the low-level details of how Vue works (I'm trying to!) so please bear with me if I'm saying stupid things, but I think this might not be as difficult, since you're already applying this PR to KeepAlive. Or this just "freezes" the first child of KeepAlive and not it's inner children in its current state? If we used a mechanism like provide/inject for this, I think it might be doable and efficient.

As for the naming thing I said above: I think a good naming would be <Transition freeze ...rest_of_props> to avoid breaking changes (in case this is not considered a bug, although the Keep-Alive thing is considered now, even if in Vue 2 it was the same thing 😁)

This API might also make it possible to create a Freezer component, which can simplify v-memo usage for some cases even further. Not sure if you want to pursuit my Transition RFC thing further , but either way I'm very grateful for your work on this, which hopefully helps me tackle my own RFC (which I'm surprised by the way that didn't get any traction, since I think it's a problem everybody faced at some point!) in a much simpler way. In either case, I'll be tracking this and providing (hopefully constructive!) feedback.

@Alfred-Skyblue
Copy link
Member Author

@ferferga My expression may have been unclear. What I meant is to pause only the components that affect the Transition animation, without recursively pausing all child components. In other words, I will only invoke scope.pause on a specific component that influences the animation. Conversely, if we perform recursion, it would pause the EffectScopes of all child components.

@ferferga
Copy link
Contributor

ferferga commented Jan 9, 2024

@Alfred-Skyblue Great then! That's exactly what I had in mind for my RFC and I think it should be enough to solve all the quirks demoed in the different playgrounds.

I thought you meant that only pausing the first child node of Transition would not be enough and it had to be done recursively.

Copy link
Contributor

@ferferga ferferga left a comment

Choose a reason for hiding this comment

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

I also think this can be refactored into a single toggle function that simply toggles between them. That toggle function could also be exposed in my opinion.

packages/reactivity/src/effectScope.ts Show resolved Hide resolved
packages/reactivity/src/effectScope.ts Show resolved Hide resolved
@yyx990803 yyx990803 merged commit 267093c into vuejs:minor Aug 2, 2024
7 checks passed
@Alfred-Skyblue Alfred-Skyblue deleted the feat/effect/pause-resume branch August 2, 2024 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants