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 maybe be broken with nested computed and vue-router #10236

Open
CamilleDrapier opened this issue Jan 30, 2024 · 10 comments
Open

Reactivity maybe be broken with nested computed and vue-router #10236

CamilleDrapier opened this issue Jan 30, 2024 · 10 comments

Comments

@CamilleDrapier
Copy link

Vue version

3.4.15

Link to minimal reproduction

https://github.com/CamilleDrapier/vue-test/

Steps to reproduce

  • checkout the provided repo
  • run npm install
  • run npm run dev
  • open your favorite browser on the provided local URL
  • open the web console of your browser
  • Click on the Unknown button/tab

What is expected?

  • A log with "To the unknown~" should appear (i.e.: when unmounting the page, the application should detect that the new route is not a "isAnyRoute" anymore).
    • This log should appear each time when the route is transitioned between "home" and "unknown" afterward.

What is actually happening?

  • No log with "To the unknown~"

System Info

System:
    OS: Linux 6.5 KDE neon 5.27 5.27
    CPU: (12) x64 Intel(R) Core(TM) i7-9850H CPU @ 2.60GHz
    Memory: 12.17 GB / 30.97 GB
    Container: Yes
    Shell: 5.8.1 - /usr/bin/zsh
  Binaries:
    Node: 20.11.0 - ~/.nvm/versions/node/v20.11.0/bin/node
    npm: 10.2.4 - ~/.nvm/versions/node/v20.11.0/bin/npm
  Browsers:
    Chrome: 121.0.6167.85
  npmPackages:
    vue: ^3.4.15 => 3.4.15

Any additional comments?

This repo comes with a couple of extra branches:

  • vue-3.3.13 that tries to confirm that the same code works as expected with Vue 3.3.13 (please run npm install and rerun your server after switching branches)
  • is-home which uses a slightly less-nested isHome computed, which seems to work as expected.
  • no-watch, which comments out a watcher on the "offending isAnyRoute" computed. The application seems to work as expected.

This problem could be related to other reactivity issues that were reported on the 3.4.X branch, but it is a bit difficult for me to say if it is a duplicate of another issue. I could confirm that the same problem happens as well in 3.4.13, so maybe it is different than #10185 but might still be the same "narrow edge case" that is mentioned over there.

Sorry in advance for this complex use case; if anything it might not affect many users, and it can probably be worked around if need be~ 🙇

@ikkkp
Copy link

ikkkp commented Jan 30, 2024

image
'To the unknown~'Shouldn't it be triggered when the value of isAnyRoute.value is false? I don't see any exception here. Is it a problem with the Linux system?

@CamilleDrapier
Copy link
Author

Thanks for looking and helping with this matter!

I doubt that this is an Operating System specific problem, as I can reproduce the problem with the repository I provided on my Windows 10 computer. Also, the problem I'm facing with my real-life application has been reported/confirmed by Linux, Windows, and Mac users.

Sorry for my previous lack of explanation, the problem here is that the isAnyRoute.value is true, while the internal log for the computed isAnyRoute reports that its constituent computed properties (isHome, isAbout) are both false. Thus accessing isAnyRoute.value should be false after isAnyRoute has been computed for the last time.

  • When navigating from the "home" page to the "unknown" page, you can see the following logs:
    • isAnyRoute false false, this is expected because the new route is neither 'home' nor 'about', and the computed isAnyRoute is being calculated because the route it relies on has changed (and because the result is displayed n the page).
    • Unmounted Home - isAnyRoute: true, this is unexpected, because the new return value that was just previously calculated is false
  • If you try the same code with version 3.3.13 (by using the branch vue-3.3.13 on the provided repo for example, and of course run npm install and npm run dev again) you can see:
    • isAnyRoute false false, again: this is expected/normal - but it is triggered twice, so maybe in the latest version there is an optimization that prevents this re-calculation.
    • Unmounted Home - isAnyRoute: false, this time the computed value from the "using component" has the expected false value.
    • To the unknown~, since the previous value is false, this log appears.

Hopefully, this helps clarify my previous report; sorry for the convoluted reproduction pattern, I tried trimming as much as I could!

@ikkkp
Copy link

ikkkp commented Jan 31, 2024

The double triggering of isAnyRoute false false might be due to the combination of <p>{{ useRouteHelpers().isAnyRoute }}</p> and the onUnmounted hook. When I attempted to comment out the watch, the reactivity system behaved normally. I hope this insight can provide some ideas for others trying to troubleshoot the issue.
https://stackblitz.com/~/github.com/ikkkp/vue-test-main
image

@yyx990803
Copy link
Member

FYI Calling composables inside the render function is wrong usage and should be avoided. Composables should only be called once per component, inside setup() or <script setup>.

@CamilleDrapier
Copy link
Author

CamilleDrapier commented Feb 7, 2024

Thanks for the additional insight, and sorry for this improper use of composables in my example!

I have updated my example repository which produces the small problem I mentioned in my original post. It should not have calls to composable inside the render/template anymore. And I also got rid of irrelevant warnings. Hopefully, this makes it easier to understand the problem. 🙇

@yyx990803
Copy link
Member

My guess is this is because in 3.4, chained computed becomes lazy evaluated - it doesn't compute until it is needed, but by the time in onUnmounted, the first-level computed effects are already stopped and never triggered the chained computed to become dirty. I will need to look a bit deeper to confirm this though.

/cc @johnsoncodehk

johnsoncodehk added a commit to johnsoncodehk/core that referenced this issue Feb 7, 2024
@Doctor-wu
Copy link
Contributor

@yyx990803 @johnsoncodehk I believe this is a timing issue about the computed disposed before the onUnmounted cb invoke and I think it is reasonable.

  1. Mount HomeView and get isAnyRoute.

  2. Click unknown and cause HomeView unmount, but scope stop will occurred before invoke um hooks.
    Stop scope will stop the effect registered in it as well, here include isAnyRoute.
    When the stop happened in isAnyRoute, it will cleanup the deps of isAnyRoute.
    CleanShot 2024-02-07 at 22 45 24@2x

  3. Unmounted cb invoke

  console.log('Unmounted Home - isAnyRoute:', isAnyRoute.value); // isAnyRoute is MaybeDIrty
  if (!isAnyRoute.value) console.log('To the unknown~');

HereisAnyRoute is MaybeDirty but it was cleaned up though and will return NotDirty, so it will return cached value.
CleanShot 2024-02-07 at 22 59 22@2x

So, @CamilleDrapier I think it's not a good practice to use effects in onUnmouted hooks, since it may be disposed, and I believe it will work in onBeforeUnmount hooks in your case.

@Doctor-wu
Copy link
Contributor

Oops, found Johnson has a PR for this one after I commented 😂
anyway, onBeforeUnmount is at least a temporary workaround

@bryandonmarc
Copy link

Might be related, but we also encountered this issue on our in-house UI Component Library where a component was provided router link props.

Here is the behavior minimally reproduced:

On Vue 3.3.13 it works: playground
On Vue 3.4.19 it doesnt (resulting in Uncaught (in promise): Cannot read properties of undefined (reading 'resolve')): playground

(I had to use Vuetify Playground bc Vue SFC Playground wasnt behaving as expected no matter which version 3.3/3.4)

yyx990803 added a commit that referenced this issue Feb 23, 2024
yyx990803 added a commit that referenced this issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants