Avoid computing computed with storeToRefs()

[Ericlm]

Reproduction

https://github.com/Ericlm/pinia-computed

Steps to reproduce the bug

Since version 2.2.5, computed values are evaluated at the start even if they're not displayed (when using storeToRefs).

It may be related to 254eec7

Expected behavior

The computed should only be evaluated when it is actually used.

Actual behavior

The computed is evaluated even if its value is displayed inside a v-if="false".

Additional information

No response

[posva]

This seems to be inherent to Vue's toRef(). The same behavior exists in regular Vue. If you want to avoid computation, you can directly reference the store with store.doubleCount or pass () => store.doubleCount.

A possible improvement here would be to change the current implementation related to HMR in order to keep using the old version in storeToRefs() that references the raw store object (thus not triggering reactivity). Contribution welcome.

Edit: I'm not completely sure we want to revert this change because if this behavior exists in current Vue, it's more natural to keep it. That being said I would like to have real world examples of why this change affects you

[sceee]

I understand that this might be according to Vue's toRef(), but nevertheless this seems to be like a pretty big change in behavior.
In my case, it seems to "break" a lot of stuff as it just behaved different until now and therefore was used in that way.

@posva as I'm not sure I understood your tips regarding avoiding computation correctly (and I can unfortunately also not view your "play" link as it throws an exception InvalidCharacterError: Failed to execute 'atob' on 'Window': The string to be decoded is not correctly encoded. which seems to be a general vue play error at the moment):

What exactly do you mean with "you can directly reference the store"?
To use this

const { myComputedProp } = myStore

instead of

const { myComputedProp } = storeToRefs(myStore)

?
But this is then no longer reactive, is it?

Or what do you mean with your other mentioned fix "pass () => store.doubleComputed"?
This:

const myComputedProp = computed(() => myStore.myComputedProp)

?

Thanks.

[posva]

I updated my comment with a fixed link. In your case, it seems like you can simply not use storeToRefs() at all. The real use case for it is to pass the extracted ref to another composable. Most of the time, these composables also accept a getter function () => store.computed, which is why I'm suggesting the getter

In my case, it seems to "break" a lot of stuff as it just behaved different until now and therefore was used in that way.

Interested in learning more and available to help as well!

[marr]

Most of the time, these composables also accept a getter function () => store.computed, which is why I'm suggesting the getter

I'd like to understand this more thoroughly. In the app I'm working on, we seem to use storeToRefs all over the place. I use computed quite often and thought I would have to use the .value of the transformed (via storeToRefs) ref. If this is not the case, can you tell us how that works? Since computed depends on reactive values, how would it work just referencing store.foo since that doesn't seem to be reactive?

[posva]

store.foo is reactive because store is a reactive object

[DaleMckeown]

I opened #2829 earlier but it was closed down, which is fine as I was still struggling with how to replicate the problem in the play link.

I've spent 2 days debugging this issue in Pinia, and I finally understand what was happening in my project.

One store was breaking my app, with a completely white screen and lots of Vue warn errors:

 Slot "default" invoked outside of the render function: this will not track dependencies used in the slot. Invoke the slot function inside the render function instead. 

This specific store had some computed properties that were referencing other computed properties, like so:

export const useDataStore = defineStore('data', () => {

  const loadingData = ref(true);
  const data = ref<User[]>();

  async function loadData() {
    try {
      loadingData.value = true;

      setTimeout(() => {
       data.value = [
          { id: 1, name: 'Alice', userType: 'student', age: 20 },
          { id: 2, name: 'Bob', userType: 'student', age: 22  },
          { id: 3, name: 'Charlie', userType: 'teacher', age: 40  },
          { id: 4, name: 'Tina', userType: 'student', age: 21  },
          { id: 5, name: 'Steven', userType: 'teacher', age: 45  },
          { id: 6, name: 'Abdul', userType: 'student', age: 19  },
          { id: 7, name: 'Yet', userType: 'teacher', age: 50  }
        ];
      }, 2000);
    }
    catch (error) {
      console.log(error);
    }
    finally {
      loadingData.value = false;
    }
  }

  const allStudents = computed(() => {
    return data.value.filter(user => {
      return user.userType === 'student';
    });
  });

  const studentsUnder20 = computed(() => {
    return allStudents.value.filter(user => {
      return user.age < 20;
    });
  });

  return { loadingData, data, allStudents, studentsUnder20, loadData }

});

Pinia Playground Link

In 2.2.4 and below, the above was perfectly fine. In 2.2.5 and above, my computed properties have to use optional chaining, otherwise the store breaks due to that early evaluation:

const allStudents = computed(() => {
  return data.value?.filter(user => {
    return user.userType === 'student';
  });
});

const studentsUnder20 = computed(() => {
  return allStudents.value?.filter(user => {
    return user.age < 20;
  });
});

I also feel like the behaviour should not have been changed without warning. At the very least, docs should be updated to reflect the new behaviour.

[skirtles-code]

I just encountered someone having this problem on Vue Land.

Here's a reproduction of the problem they were having:

• Playground

They were using propValue conditionally in the template (inside a v-if), but now storeToRefs throws even without accessing propValue.value.

This works fine with 2.2.4 and earlier, but fails with 2.2.5. As noted by others, this seems to have been caused by #2795.

When storeToRefs calls toRef internally it eventually gets to this code in Vue:

function propertyToRef(
  source: Record<string, any>,
  key: string,
  defaultValue?: unknown,
) {
  const val = source[key]
  return isRef(val)
    ? val
    : (new ObjectRefImpl(source, key, defaultValue) as any)
}

In Pinia 2.2.4 and earlier the source object is the raw, unwrapped store object, so accessing source[val] is just grabbing the ComputedRefImpl for the getter out of that object. All works well.

With 2.2.5 the source object is now a proxy, so accessing source[key] tries to unwrap the ComputedRefImpl, causing the error.

Would the fix in Vue core be something like this?

function propertyToRef(
  source: Record<string, any>,
  key: string,
  defaultValue?: unknown,
) {
  if (!isProxy(source)) {
    const val = source[key]
    if (isRef(val)) {
      return val
    }
  }
  return new ObjectRefImpl(source, key, defaultValue) as any
}

If source is a proxy (i.e. reactive or readonly) then it doesn't seem necessary to do the isRef check, as proxies will always unwrap them anyway.

Update: The code I suggested above isn't quite right, as it won't handle shallowReactive/shallowReadonly correctly. Handling those correctly is a bit fiddly, but I'm working on it.

[posva]

@DaleMckeown In your case, the fix is very simple: const data = ref<User[]>([]) pass that initial value. You will notice that you get a TS error

[posva]

@skirtles-code I marked this upstream but I think we could and should handle getters and computed differently in storeToRefs() so they are still lazily evaluated. Keeping the HMR working is also needed though.

[DaleMckeown]

@DaleMckeown In your case, the fix is very simple: const data = ref<User[]>([]) pass that initial value. You will notice that you get a TS error

Good point, cheers for the hint.

[kubajmarek]

This causes some issues for me, as I throw an error on one getter in case some properties aren't yet set. Now most storeToRefs on that store throw that error, even though the getter remains unused.

Let me know if you think this is an anti-pattern.

[posva]

Yes, that seems like an anti pattern: a computed shouldn’t throw

[ezio-melotti]

I'm seeing the same issue with getters too. Now the getters seem to be evaluated as soon as I import the store, and that causes errors because of variables in the getters that are not yet initialized.

[posva]

Now the getters seem to be evaluated as soon as I import the store

@ezio-melotti I suppose you mean when using storeToRefs(). getters are computed properties, so this should be the same issue. Usually you catch this kind of errors with types. You can temporarily enable // @ts-check at top of the file to fix these issues and then remove the comment

[ezio-melotti]

@posva, thanks for your reply and for looking into this!
The fix you applied in 67d3109 should be included in v2.2.8, but even with that version I'm getting the same error. Is this expected?

I understand that this change in behavior might have simply revealed already-existing errors in my code -- which is great! -- but it's nonetheless an unexpected change, especially for a bug-fix release. I'm also somewhat concerned about how the change might affect other computed/getters that are now evaluated earlier without failures, but might have side-effect that will cause issue later on.

[posva]

computed (and getters) should not have any side effects, that’s what watchers are for
I can’t check your personal project but you can either book a cal or provide a boiled down repro