-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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(testing): allow writable pinia getters in Vue 2 #1201
base: v2
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for pinia-official canceled.
|
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be
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
There was a problem hiding this comment.
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?
return internalValue !== undefined ? internalValue : (isVue2Getter ? value : value.value) | |
return internalValue !== undefined ? internalValue : (isVue2Getter ? options.getters![key](store): value.value) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Hey @posva sorry about the radio silence, got busy these last couple weeks. Based on your feedback it sounds like we should close this PR but maybe leave the issue? Would you agree? |
It’s up to you: it’s okay if you want to keep it open for later |
So is there a known workaround to mock getters in Vue2? So far I've been resorting to supplying the required state data to have the getters return as expected. But it's fairly verbose at times for complex getters. |
also interested in mocking getters in vue 2. if i mock the state then that means im also testing store code, which isnt ideal for component unit tests. |
This is how I've been getting around it IF you're writing getters in pinia with Options API. I mocked the utilization to be fairly similar to how initialState is structured (storeId key, an object of getters as value). Example below it. /**
* A Pinia plugin for mocking getters in component tests.
*
* @param {object} gettersMap
* @returns {Function}
*/
export const initialGetters = (gettersMap) => ({ store, options }) => {
if (gettersMap[store.$id]) {
Object.keys(options.getters).forEach((getter) => {
Object.defineProperty(store, getter, {
get: () => gettersMap[store.$id][getter] || options.getters[getter],
});
});
}
}; // Example.spec.js
mount(Example, {
pinia: createTestingPinia({
plugins: [
initialGetters({
myStore: {
someGetter: jest.fn(),
anotherGetter: jest.fn(() => 12),
},
}),
],
}),
}); |
Any news ? |
here a workaround: #1200 (comment) |
Description:
This allows Pinia store getters in Vue 2 apps to be reassigned as shown in the test here.
Related Issues:
Close #1200
Testing Notes:
I wasn't able to write a test for this directly into the pinia/testing repo due to the fact that I would need to use the
vue-demi switch 2 vue2
npm script, which requires me to install vue/composition api, which then causes a number of peer dependency issues. I talk about this further in the Issue attached.Code Notes:
I'm not married to the nested ternary, and I don't know if there's a precedence for optional chaining, so I'm happy to adjust any of these.