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 regression in 3.14.15 #10185

Closed
basvanmeurs opened this issue Jan 22, 2024 · 15 comments · Fixed by #10187
Closed

Reactivity regression in 3.14.15 #10185

basvanmeurs opened this issue Jan 22, 2024 · 15 comments · Fixed by #10187

Comments

@basvanmeurs
Copy link
Contributor

basvanmeurs commented Jan 22, 2024

Vue version

3.14.15

Link to minimal reproduction

https://play.vuejs.org/#eNqdVE1z0zAQ/StbX5IMiU0pHJqmpcB0hnIApjDTiy6uvbHVypJHkp12Mv7vrOSPuGlggBwy1r79ePtWq23woSzDusJgGaxMonlpwaCtygsmeVEqbWELGtdzMHkshNrcuO9EFWVlMYUG1loVMKEEEyaZTJQ0FgqTwbmLmk4+IwXBrdIiPZrMvIuIjYFriwVsmQT61Z3z69kZk80uDScfQ9iu8tR59DCh5RuCezLT6QzOL/qkUQQ3mGk0hisJeWzAKkgVbLjNweo4eeAyA5ujpBPPMtR0nsMdJnFlEBKNsXUenqiqrOEpglq7iF37Bbc8iy1ScjKXWt0JLMKWgEALCdHzXYR1LCok8g7ha5geJbOeqfs5R4kbX8032QOjaNdqh3iR3IemUWkJSVgPFZqxRkLFKfFs9V3HwuBzBenvsICeYxvdZp71tVggFQvOnMC3pCVJQ81zMyc8TiyvuX2CjdIPxuvQBflZdW28pxRPaFgAyz7ZHmuaWiUOMHN8Ozbem+7ZIA0LRjVcYnjV5RmUYXLcEMVY3dn/OdHugr6YsDv14yDw9PT0/0qsonYdaRHp4AIE3TQ6Aazy44vt1q9Z06wiOnkrlyQW1ItCpSjOWUA4pY4IXEWj+GAetJu9KOIyvDdK0u77obMOoNks+9vJAtptd2ZBbm1pllGUpJLCqAavdSjRRrIsoktyi3QlLS9wkari8iR8Gx6/i1Ju7NgeoikWd1ptDGrKwoL5qE5Exhr1QqNMUaP+27p7Yc9q72Ev6rvytE8NyWINzXXNsz1R3C3kAvW30tJb8lwc/zJ98TZ3m4ZmkhyThwP2e/PYNvWdBu6YjQSwsc7QtvDVj6/4SN8DSEOtRDeI34A3aJSoHMfW7WMlU6I98vNsr/2M6W37aa4eLUr3QA5EvRre3w/k0x9a39E9CU8GFZtfULwgAQ==

Steps to reproduce

  1. Run the link

What is expected?

Input should show temp2.value: yes because item.v.value is updated which should trigger temp2, test and the watcher.

What is actually happening?

Input shows temp2.value: no

System Info

No response

Any additional comments?

In vue 3.14.12 a similar problem was introduced, which was fixed in 3.14.13. Then in 3.14.15 the same problem was re-introduced but for a more narrow edge case. Faulty commit: c2b274a

@Doctor-wu
Copy link
Contributor

That's the expected behavior, because computed are now only re-calculate and trigger effects when their return values changed.
In your reproduction, the test returned value never changed, so the test didn't trigger effects are desirable behavior.
You can refer to Evan's explaination in the earlier issue.

@basvanmeurs
Copy link
Contributor Author

basvanmeurs commented Jan 22, 2024

Thanks but I think that the refered issue (and explanation) refers to returning an object.

test returned value changes from "no" to "yes", which are both primitive values.

I think that this is a regression caused by multiple levels of reactivity and tracking/triggering from within the same computed. Note that this worked up to version 3.4.14.

As a sidenote, I implemented a good part of the original reactivity system before the 3.4 refactoring.

@Doctor-wu
Copy link
Contributor

Doctor-wu commented Jan 22, 2024

I got you now, sry for my misunderstanding. But I still think this is the desirable behavior.
CleanShot 2024-01-23 at 00 06 15@2x
There's a similar test case in this @johnsoncodehk's PR
As a sidenote, I fixed a regression issue which was released in 3.4.14 and Johnson re-fix it in 3.4.15.

@Doctor-wu
Copy link
Contributor

CleanShot 2024-01-23 at 00 12 24@2x
And the dirtyLevel are indeed correct

@basvanmeurs
Copy link
Contributor Author

basvanmeurs commented Jan 22, 2024

I agree that the computed is dirty immediately after getting it. And I did notice the dirtyLevel still being on 2. But this did usually lead to a re-evaluation when accessing it.

But these are just internals.

A computed should always produce the latest valid value, and trigger its dependencies whenever it changes. This is how it always worked up to now. Imho anything different should be regarded as a regression. Just like #10114 was regarded as a regression. The fix just didn't handle this more complex edge case.

The problem lies in the fact that temp2 was evaluated within test. When loaded was enabled, this caused temp2 to be evaluated. After evaluation, it ended up dirty while the 'outer' computed test did not. This end result is undesirable as updates to the inner computed will not trigger the outer computed. I think that a computed, after ending up dirty after evaluation, should automatically re-trigger its dependencies.

@basvanmeurs
Copy link
Contributor Author

Or alternative solution: re evaluate computeds until they end up not dirty.

@Doctor-wu

This comment was marked as outdated.

@Doctor-wu

This comment was marked as outdated.

@Doctor-wu

This comment was marked as outdated.

@Doctor-wu
Copy link
Contributor

Wait... I think maybe this should work like this case?
The difference is the c2 is MaybeDirty.

@Doctor-wu
Copy link
Contributor

I agree with @basvanmeurs that this is a reactivity regression now.

@basvanmeurs
Copy link
Contributor Author

Thanks @Doctor-wu

You made me realize that this is an anti-pattern in my code. But it would be nice if we could support recursive triggering and fix this one.

The reactivity refactoring in 3.4 could actually work in favor of this. If a computed retriggers itself we now have the chance to immediately re-evaluate as part of the value getter, before even triggering deps! Of course we needto watch out for infinite recursion and console.warn in case it happens..

@basvanmeurs basvanmeurs changed the title Reactivity regression in 3.14.13 Reactivity regression in 3.14.15 Jan 22, 2024
@johnsoncodehk
Copy link
Member

It should be fixed by #10187. It would be helpful if you could provide a minimal test case.

@Doctor-wu
Copy link
Contributor

Doctor-wu commented Jan 23, 2024

It should be fixed by #10187. It would be helpful if you could provide a minimal test case.

I can provide a minimal test later Already done

@basvanmeurs
Copy link
Contributor Author

The reactivity refactoring in 3.4 could actually work in favor of this. If a computed retriggers itself we now have the chance to immediately re-evaluate as part of the value getter, before even triggering deps! Of course we needto watch out for infinite recursion and console.warn in case it happens..

A PR that explores this opportunity: #10189

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants