Skip to content

Commit

Permalink
fix: avoid immediate computing with storeToRefs
Browse files Browse the repository at this point in the history 
Fix #2812
  • Loading branch information
@posva
posva committed Nov 28, 2024
1 parent ea14e53 commit 67d3109
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 2 deletions.
15 changes: 14 additions & 1 deletion packages/pinia/__tests__/storeToRefs.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { describe, beforeEach, it, expect } from 'vitest'
import { describe, beforeEach, it, expect, vi } from 'vitest'
import { computed, reactive, ref, ToRefs } from 'vue'
import { createPinia, defineStore, setActivePinia, storeToRefs } from '../src'
import { set } from 'vue-demi'
Expand Down Expand Up @@ -190,6 +190,19 @@ describe('storeToRefs', () => {
expect(double.value).toEqual(1)
})

it('does not trigger getters', () => {
const n = ref(0)
const spy = vi.fn(() => n.value * 2)
const store = defineStore('a', () => {
const double = computed(spy)
return { n, double }
})()

expect(spy).toHaveBeenCalledTimes(0)
storeToRefs(store)
expect(spy).toHaveBeenCalledTimes(0)
})

tds(() => {
const store1 = defineStore('a', () => {
const n = ref(0)
Expand Down
15 changes: 14 additions & 1 deletion packages/pinia/src/storeToRefs.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
computed,
ComputedRef,
isReactive,
isRef,
Expand Down Expand Up @@ -99,7 +100,19 @@ export function storeToRefs<SS extends StoreGeneric>(
const refs = {} as StoreToRefs<SS>
for (const key in rawStore) {
const value = rawStore[key]
if (isRef(value) || isReactive(value)) {
// There is no native method to check for a computed
// https://github.com/vuejs/core/pull/4165
if (value.effect) {
// @ts-expect-error: too hard to type correctly
refs[key] =
// ...
computed({
get: () => store[key],
set(value) {
store[key] = value
},
})
} else if (isRef(value) || isReactive(value)) {
// @ts-expect-error: the key is state or getter
refs[key] =
// ---
Expand Down

8 comments on commit 67d3109

@norwd
Copy link

@norwd norwd commented on 67d3109 Dec 3, 2024

Choose a reason for hiding this comment

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

@posva, I think this commit has caused an error when value is null:

Image

@posva
Copy link
Member Author

@posva posva commented on 67d3109 Dec 4, 2024

Choose a reason for hiding this comment

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

@norwd #2848 (comment)

@tvartom
Copy link

@tvartom tvartom commented on 67d3109 Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This breaks typescript code like this, that have worked before:
(Broke our app when bumped to 2.2.8)

interface UserStore {
  userList: string[]
  user?: string
}

export const useUserStore = defineStore('user', {
  state: (): UserStore => ({
      userList: [],
      // user is omitted, needs to be `user: undefined` after this commit, to be able to use storeToRefs
  }),
})

I guess if(value && value.effect)would fix it.

@posva
Copy link
Member Author

@posva posva commented on 67d3109 Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@tvartom You do need to declare all state properties. They can start as undefined but cannot be omitted. Adding user: undefined and user: string | undefined (in the type) is the right approach in your case 👍

@norwd
Copy link

@norwd norwd commented on 67d3109 Dec 4, 2024

Choose a reason for hiding this comment

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

@posva, Thank you for linking to #2848, that's cleared up how we can mitigate the issue. However, this has caused a massive disruption to our system as it is a breaking change in behaviour. I agree with @tvartom and @Jv-Juven, this should be fixed to restore the previous behaviour.

@maxfrees
Copy link

Choose a reason for hiding this comment

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

This is very important destructive behavior and this should be fixed to revert to the previous behavior @posva

@maxfrees
Copy link

Choose a reason for hiding this comment

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

let breakpointDecoration: EditorType.IEditorDecorationsCollection | undefined;

let breakpointHitDecoration: EditorType.IEditorDecorationsCollection | undefined;

Sometimes you need to store data that is not a ref

@Ben-Pfirsich
Copy link

Choose a reason for hiding this comment

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

This also somehow breaks the tests in our code.
The element in the the store looks like this:
const selectedLink: Ref<Link | undefined> = ref<Link | undefined>(undefined);
In the code it is referenced as follows:
const { selectedLink } = storeToRefs(linkStore);
In the testsetup I initialize the store like this:
createTestingPinia({ initialState: { links: { selectedLink: { ...getDefaultLink(), }, })
After Updating the Version to 2.2.8, the test fails with the following error:
Cannot read properties of undefined (reading 'effect')

Please sign in to comment.