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

A getter not updated when option store used with Vue 2 #1800

Closed
denego opened this issue Nov 14, 2022 · 16 comments
Closed

A getter not updated when option store used with Vue 2 #1800

denego opened this issue Nov 14, 2022 · 16 comments
Labels
bug Something isn't working contribution welcome has workaround The issue contains a temporary solution to get around the problem vue 2.x Specific to Vue 2 usage

Comments

@denego
Copy link

denego commented Nov 14, 2022

Reproduction

https://github.com/denego/piniajs-vue2-bug-report

Steps to reproduce the bug

Clone and start example project

git clone https://github.com/denego/piniajs-vue2-bug-report
cd piniajs-vue2-bug-report
npm i
npm run serve

Open http://localhost:8080
Click test2 link

Expected behavior

number2Getter value matches number2 value

Actual behavior

number2Getter value stays the same

Additional information

This project has two simple pinia stores, each has one number (number1 and number2), one getter (number1Getter and number2Getter) returning the number, and one action (updateTest1 and updateTest2) to increment the number.

There are two pages /test1 and /test2 each rendering TestCmp.vue. The component renders numbers and getters from both stores, and calls updateTest1 and updateTest2 on mount.

Initial render is good. However, if you click on test2 link to navigate, the mount hook of TestCmp.vue will be called, and number1, number1Getter, number2 will be incremented, but number2Getter which just returns number2, will not be updated.

Also interesting, if you place number2 and number2Getter before number1 and number1Getter in TestCmp.vue, then number1Getter will not be updated.

@posva posva added bug Something isn't working contribution welcome has workaround The issue contains a temporary solution to get around the problem vue 2.x Specific to Vue 2 usage labels Nov 14, 2022
@posva
Copy link
Member

posva commented Nov 14, 2022

Not sure where this could come from TBH, so any contribution is welcome. Note this could also be an upstream bug.

As a workaround, using setup() seems to not show the issue.

@denego
Copy link
Author

denego commented Nov 14, 2022

setup syntax works indeed.
I just didn't realize I can use composition API with Vue 2.
Thanks!

@denego denego changed the title A getter not updated when pinia used with Vue 2 and vue-router 3 A getter not updated when option store used with Vue 2 Nov 14, 2022
@a-kriya
Copy link

a-kriya commented Jan 17, 2023

@posva Is this bug unlikely to be fixed officially since it's an Options API-only issue? Any pointers on where the issue may be?

@emahuni
Copy link

emahuni commented Jan 20, 2023

I have figured why this is happening. This is coming from vue-demi. I didn't go through the rabbit hole to see why, since Vue 2.7 doesn't require it anymore.

This is causing a lot of problems in apps and should be solved. Sometimes the bug is not that apparent and causes a lot of hours of debug. So what i have done is isolate the problem towards the store and then landed in Pinia. That's when I noticed that reactive data is working as usual, but the moment you use computed/getter with data in/from the store, then you have issues somewhere.

The only way those getters could work was if I do this instead of the other:

<template>
  works: {{ theGetter.value }}
  doesn't work: {{ theGetter }}
</template> 
<script> 
export default { 
   // ... setup() use store blah blah boilerplate
  methods: {
    aMethod () {
        // This is not happening with other composition api refs/reactives
       const x = this.theGetter.value ? 'x' : 'y' // this works
       const x = this.theGetter ? 'x' : 'y' // this doesn't
    }
  }
}
</script>

I noticed in Pinia code that it uses Vue-demi & Vue-composition-api packages to patch Vue 2.6 code. This is an old approach since Vue2.7 is now compatible without using those packages.

I then forked Pinia and removed vue-demi & vue-composition-api from the pinia package and patched where they were necessary and it works without hacks.

The only reason why demi exists is to shim the following imports:

import { set, del, isVue2 } from 'vue-demi'

Solution:

  • set: used to go around vue2 reactivity caveats, so I used the app instance in pinia._a for Vue2 since it is available in every instance: pinia._a.$set and removed the import. I used store[key]=... where Vue 3 is used and required similar code.
  • del: same as above, so I used pinia._a.$delete and removed the import. I used delete store[key] where Vue 3 is used and required similar code.
  • isVue2: I imported version from 'vue' and created a little constant const isVue2 = /^2/.test(version) to do the same thing

The above fixes need testing and more work from those who are more knowledgeable of Pinia, but that is why this issue is happening. I am not sure if this is the right thing to do to fix this.

Note that these packages are causing those reactivity issues though they are no longer necessary. May I propose that Pinia simply requires ^Vue@2.7 for Vue2 users and remove these dependencies completely.

@emahuni
Copy link

emahuni commented Jan 20, 2023

I think Vue 3 should have just left some of these redundant code exports for the sake of Vue2 compatibility. Vue 3 has no set or del, created etc, I understand why, but it makes it harder to create compatible plugins and software that run on either 3 or 2. This issue in Pinia is a clear example of this.

@a-kriya
Copy link

a-kriya commented Jan 20, 2023

@emahuni I tried your forked repo with the changes, and it doesn't build:

image

@emahuni
Copy link

emahuni commented Jan 20, 2023

@emahuni I tried your forked repo with the changes, and it doesn't build:

image

oh, yah, that was the initial thing when I was trying to see if removing the packages changes anything. I eventually cloned it to my machine when I noticed there were a lot of things that needed editing. Ran into issues with reactivity on compatibility btwn 2 and 3, when there is no pinia._a, that solution breaks and has a few reactivity issues. Wanted to push it when I have cleaner code and works fine.

But the issue is around those functions. As I said, I think vue3 needs to keep these little functions to maintain plugin compatibility because nothing builds when using v3 since those exports don't exist, even though everything else maybe fine. Am currently trying to port them into Pinia since they are just 2 of them.

For now, the getter.value workaround works for me.

@fazulk
Copy link

fazulk commented Jun 8, 2023

Noticing the same bug here using vue 2.7.14, with script setup .

When the store is options api, getters work "sometimes". and when using the composition store, getters work as intended. Will be able to provide more context about what that sometimes means shortly..

@gcaaa31928
Copy link
Contributor

I think I have identified the root cause. First, in set(pinia.state.value, id, state ? state() : {}), it's highly likely to trigger the renderTriggered hook of a certain component's lifecycle. Then, due to the usage of the previous currentInstance in callHook, the scope of the previous instance is activated after the callHook is completed. As a result, the computedGetters in this section are all collected into the incorrect scope mentioned earlier.

Subsequently, when the component is being destroyed, _scope.stop is called. At this point, all the computed properties inside computedGetters are teardown, causing the getters to become non reactive.

This issue's correct solution might involve addressing it from the Vue callHook. If possible, I'd be glad to assist in preparing a pull request for the fix.

@SchDen
Copy link

SchDen commented Aug 22, 2023

Really need this fix 🙏

@gcaaa31928
Copy link
Contributor

@posva
I have submitted the pull request. Please take a look. Thank you.

@brian428
Copy link

brian428 commented Oct 6, 2023

Can this PR please be merged? This is a major issue for anyone trying to migrate large existing apps to Vue 3. The actual code change is only 6 lines, and it includes a spec, so I'm not sure what the holdup is here.

@brian428
Copy link

brian428 commented Oct 6, 2023

I should have tagged @posva

@gcaaa31928
Copy link
Contributor

If the Vue issue cannot be merged yet, would it be possible to consider this workaround pull request in the meantime?

#2452

@gcaaa31928
Copy link
Contributor

@posva For those who are using Vue 2, it would offer a better user experience. Please consider merging it.

@posva
Copy link
Member

posva commented Oct 23, 2023

@gcaaa31928 Thanks for the Vue PR, it got merged and released:

@posva posva closed this as completed Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working contribution welcome has workaround The issue contains a temporary solution to get around the problem vue 2.x Specific to Vue 2 usage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants