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.

Sign up for GitHub

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

Jump to bottom

fix(testing): allow writable pinia getters in Vue 2 #1201

Open
wants to merge 1 commit into
base: v2
Choose a base branch
from
Open

fix(testing): allow writable pinia getters in Vue 2 #1201

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/testing/src/testing.ts
View file
Open in desktop
Expand Up @@ -197,17 +197,19 @@ function isComputed<T>(
return !!v && isRef(v) && 'effect' in v
}

function WritableComputed({ store }: PiniaPluginContext) {
function WritableComputed({ store, options }: PiniaPluginContext) {
const rawStore = toRaw(store)
for (const key in rawStore) {
const value = rawStore[key]
if (isComputed(value)) {
// Vue2 store getters are not refs and can't be found with isComputed()
const isVue2Getter = isVue2 && options.getters?.[key]
if (isComputed(value) || isVue2Getter) {
rawStore[key] = customRef((track, trigger) => {
let internalValue: any
return {
get: () => {
track()
return internalValue !== undefined ? internalValue : value.value
return internalValue !== undefined ? internalValue : (isVue2Getter ? value : value.value)
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be

Suggested change
return internalValue !== undefined ? internalValue : (isVue2Getter ? value : value.value)
return internalValue !== undefined ? internalValue : (isVue2Getter ? store[key] : value.value)

I haven't tested it but I don't think reading value will be reactive

Copy link
Author

Choose a reason for hiding this comment

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

@posva ah, you're right, the reactivity isn't retained with just value. though using store[key] causes an infinite get loop 😅 I think the correct move here would be to call the function from options.getters, no?

Suggested change
return internalValue !== undefined ? internalValue : (isVue2Getter ? value : value.value)
return internalValue !== undefined ? internalValue : (isVue2Getter ? options.getters![key](store): value.value)

Copy link
Member

Choose a reason for hiding this comment

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

Ah true about the infinite loop! The problem with options is that it’s not always there eg setup stores. I think there is an open issue in composition api regarding the detection of computed properties. We will likely need to push that forward instead

Copy link
Author

Choose a reason for hiding this comment

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

Hmmm I think I see what you're saying. Are you referring to someone using setup stores in vue 2? I wasn't aware that was possible. If that's the case then yeah, you're right. this wouldn't work for that particular scenario.

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Copy link
Author

Choose a reason for hiding this comment

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

@posva I was looking into the vue/composition-api plugin docs a little more and it looks like you merged a PR that does differentiate computed properties in composition. Since you're looking for effect in your isComputed function and I'm looking for options.getters?.[key] in isVue2Getter, that tells me that all bases are covered here. isVue2Getters couldn't be true if there were no options.getters object in the store.

I'm going to add another test to the mock project I'm using to test vue2 with the vue/composition-api plugin to confirm, but does this sound right to you?

Copy link
Member

Choose a reason for hiding this comment

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

The problem is toRaw() doesn't work in composition-api

},
set: (newValue) => {
internalValue = newValue
Expand Down