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

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

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

Conversation

heykc
Copy link

@heykc heykc commented Apr 4, 2022

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.

@netlify
Copy link

netlify bot commented Apr 4, 2022

Deploy Preview for pinia-official canceled.

Name Link
🔨 Latest commit 5be65eb
🔍 Latest deploy log https://app.netlify.com/sites/pinia-official/deploys/624b60d5fd577800099404fd

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

@posva posva added the WIP label Apr 19, 2022
@heykc
Copy link
Author

heykc commented Apr 19, 2022

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?

@posva
Copy link
Member

posva commented Apr 19, 2022

It’s up to you: it’s okay if you want to keep it open for later

@MisterIsaak
Copy link

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.

@rhysv96
Copy link

rhysv96 commented May 10, 2022

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.

@heykc
Copy link
Author

heykc commented May 10, 2022

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),
        },
      }),
    ],
  }),
});

@Graphmaxer
Copy link

Any news ?

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

Successfully merging this pull request may close these issues.

WritableComputed doesn't find getters for Vue2 testing
5 participants