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

Fix #10185, Re-evaluate effect immediately until it doesn't re-trigger itself #10189

Closed
wants to merge 4 commits into from

Conversation

basvanmeurs
Copy link
Contributor

@basvanmeurs basvanmeurs commented Jan 23, 2024

This is a proposal, or at least something to think about.

The improved reactivity system now evaluates computeds lazily. This caused issues when the computed re-triggered itself (in)directly. See #10185

Although I can follow the idea that direct recursive re-triggering is an antipattern, we must support this situation. Re-triggering may happen indirectly and in a not-so-obvious way to the user. Think of a sync watchers for example, which act immediately upon the change of a computed and may trigger other changes outside of the scope of the original computed. I think that we all agree that we need to support this.

The current solution leaves the computed 'dirty' after returning a value to the invoker (in the case of a recursive trigger). This value is already 'old' when it is being used. I think it would be more sane to re-evaluate the computed until it is no longer dirty, then return that value. This prevents the problems that have been found (so far), because, after executing the effect, it can never be dirty.

In the case of infinite recursion, we could detect this after X iterations mark the effect as NotDirty as normal but issue a warning.

I don't know if this is a better approach than fixing this issues one by one, as in the PR #10187

The change did have impact on the unit tests, but nothing unexpected. The unit tests have been updated in this PR.

The PR may impact current deployments using vue, because recursion is handled differently and effects may be triggered in a different order. I tested it on our own application, which makes complex use of reactivity. I found that we received one warning for infinite recursion, which is most likely true positives that we should take a look at. Other than that everything seemed to work perfectly as before.

@johnsoncodehk @Doctor-wu @yyx990803 any considerations on this approach?

Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 90.2 kB (-2 B) 34.3 kB (-13 B) 31 kB (+19 B)
vue.global.prod.js 147 kB (-2 B) 53.6 kB (+22 B) 47.9 kB (+68 B)

Usages

Name Size Gzip Brotli
createApp 50.3 kB (-2 B) 19.7 kB (+28 B) 18 kB (+2 B)
createSSRApp 53.6 kB (-2 B) 21 kB (+11 B) 19.2 kB (+14 B)
defineCustomElement 52.6 kB (-2 B) 20.4 kB (+6 B) 18.7 kB (+19 B)
overall 64 kB (-2 B) 24.8 kB (-1 B) 22.4 kB (-38 B)

@Doctor-wu
Copy link
Contributor

Doctor-wu commented Jan 23, 2024

I agree with warn users when they are using reactivity in a problematic / incorrect way, that is a good point.

In fact, I was confused with The computed keep dirty after their evaluation too, though I agree with that is reasonable then. So the problem is

Whether we should re-evaluate the computed if we found it's dirty after it's evaluate, or we just warn users

yyx990803 added a commit that referenced this pull request Feb 25, 2024
…list tracking (#10397)

Bug fixes
close #10236
close #10069

PRs made stale by this one
close #10290
close #10354
close #10189
close #9480
@basvanmeurs basvanmeurs closed this by deleting the head repository May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants