feat: diagnostic wrong values in plugins

Author: posva

packages/pinia/__tests__/storePlugins.spec.ts

@@ -1,7 +1,8 @@
 import { describe, it, expect, vi } from 'vitest'
 import { createPinia, defineStore } from '../src'
 import { mount } from '@vue/test-utils'
-import { App, computed, ref, toRef, watch } from 'vue'
+import { App, computed, markRaw, reactive, ref, toRef, watch } from 'vue'
+import { mockWarn } from './vitest-mock-warn'
 
 declare module '../src' {
   export interface PiniaCustomProperties<Id> {
@@ -13,6 +14,9 @@ declare module '../src' {
     globalB: string
     shared: number
     double: number
+    external: { dark: boolean } | null
+    reactiveProp: { n: number }
+    refProp: number
   }
 
   export interface PiniaCustomStateProperties<S> {
@@ -299,4 +303,44 @@ describe('store plugins', () => {
 
     expect(spy).toHaveBeenCalledTimes(1)
   })
+
+  describe('non-reactive properties', () => {
+    mockWarn()
+
+    it('warns when a plugin adds a non-reactive object property', () => {
+      const pinia = createPinia()
+      pinia.use(() => ({ external: { dark: true } }))
+      mount({ template: 'none' }, { global: { plugins: [pinia] } })
+
+      useStore(pinia)
+      expect('"external"').toHaveBeenWarned()
+      expect('is not reactive').toHaveBeenWarned()
+    })
+
+    it('warns when a plugin adds a null property', () => {
+      const pinia = createPinia()
+      pinia.use(() => ({ external: null }))
+      mount({ template: 'none' }, { global: { plugins: [pinia] } })
+
+      useStore(pinia)
+      expect('"external"').toHaveBeenWarned()
+    })
+
+    it('does not warn on markRaw, reactive, ref or primitive plugin properties', () => {
+      const pinia = createPinia()
+      pinia.use(() => ({
+        external: markRaw({ dark: true }),
+        reactiveProp: reactive({ n: 1 }),
+        // @ts-expect-error: cannot be a ref yet
+        refProp: ref(2),
+        shared: 10,
+      }))
+      mount({ template: 'none' }, { global: { plugins: [pinia] } })
+
+      const store = useStore(pinia)
+      // mockWarn() guarantees the diagnostic was not emitted for these
+      expect(store.external).toEqual({ dark: true })
+      expect(store.shared).toBe(10)
+    })
+  })
 })

packages/pinia/__tests__/storeToRefs.spec.ts

@@ -1,5 +1,5 @@
 import { describe, beforeEach, it, expect, vi } from 'vitest'
-import { computed, markRaw, reactive, ref, ToRefs } from 'vue'
+import { computed, reactive, ref, ToRefs } from 'vue'
 import { createPinia, defineStore, setActivePinia, storeToRefs } from '../src'
 import { mockWarn } from './vitest-mock-warn'
 
@@ -203,7 +203,7 @@ describe('storeToRefs', () => {
     expect(spy).toHaveBeenCalledTimes(0)
   })
 
-  it('does not crash and warns on a non-reactive null value', () => {
+  it('does not crash on a non-reactive null value', () => {
     const useStore = defineStore('null-val', () => {
       return {
         nullableItem: null as null | { id: number },
@@ -217,52 +217,8 @@ describe('storeToRefs', () => {
     expect(refs).toHaveProperty('text')
     expect(refs).not.toHaveProperty('nullableItem')
     expect(refs.text.value).toBe('hello')
-    expect('"nullableItem"').toHaveBeenWarned()
-    expect('is not reactive').toHaveBeenWarned()
-  })
-
-  it('warns on a non-reactive plain object value', () => {
-    const useStore = defineStore('plain-obj', () => {
-      return { config: { dark: true }, textRef: ref('hi') }
-    })
-
-    storeToRefs(useStore())
-    expect('"config"').toHaveBeenWarned()
-    expect('"textRef"').not.toHaveBeenWarned()
-  })
-
-  it('does not warn on markRaw non-reactive properties', () => {
-    const useStore = defineStore('raw-prop', () => {
-      return { external: markRaw({ dark: true }), text: ref('hi') }
-    })
-
-    const refs = storeToRefs(useStore())
-    expect(refs).not.toHaveProperty('external')
-    expect(refs).toHaveProperty('text')
-  })
-
-  it('warns on a non-reactive property added by a plugin', () => {
-    const pinia = createPinia()
-    // directly push because no app
-    pinia._p.push(() => // @ts-expect-error: invalid state
-    ({ external: { dark: true } }))
-    setActivePinia(pinia)
-
-    const refs = storeToRefs(defineStore('a', () => ({ n: ref(0) }))())
-    expect(refs).not.toHaveProperty('external')
-    expect('"external"').toHaveBeenWarned()
-  })
-
-  it('does not warn on a markRaw property added by a plugin', () => {
-    const pinia = createPinia()
-    pinia._p.push(() => ({ external: markRaw({ dark: true }), shared: 10 }))
-    setActivePinia(pinia)
-
-    const refs = storeToRefs(defineStore('a', () => ({ n: ref(0) }))())
-    expect(refs).not.toHaveProperty('external')
-    // primitives are skipped silently too
-    expect(refs).not.toHaveProperty('shared')
-    expect(refs).toHaveProperty('n')
+    // mockWarn() asserts storeToRefs() stays silent: the warning is emitted when
+    // the property is added by a plugin, not here
   })
 
   tds(() => {

packages/pinia/src/store.ts

@@ -693,33 +693,42 @@ function createSetupStore<
 
   // apply all plugins
   pinia._p.forEach((extender) => {
+    const extensions = scope.run(() =>
+      extender({
+        store: store as Store,
+        app: pinia._a,
+        pinia,
+        options: optionsForPlugin,
+      })
+    )!
+
     /* istanbul ignore else */
     if (__USE_DEVTOOLS__ && IS_CLIENT) {
-      const extensions = scope.run(() =>
-        extender({
-          store: store as Store,
-          app: pinia._a,
-          pinia,
-          options: optionsForPlugin,
-        })
-      )!
       Object.keys(extensions || {}).forEach((key) =>
         store._customProperties.add(key)
       )
-      assign(store, extensions)
-    } else {
-      assign(
-        store,
-        scope.run(() =>
-          extender({
-            store: store as Store,
-            app: pinia._a,
-            pinia,
-            options: optionsForPlugin,
-          })
-        )!
-      )
     }
+
+    // Check properties that are not properly configured. We check the values
+    // as the plugin returned them: once assigned to the store, a `reactive()`
+    // value is unwrapped and indistinguishable from a plain object.
+    if (__DEV__) {
+      for (const key in extensions) {
+        const value = (extensions as any)[key]
+        // `null` is included (typeof null === 'object'). refs, reactive objects,
+        // primitives, functions and markRaw() values are skipped.
+        if (
+          typeof value === 'object' &&
+          !isRef(value) &&
+          !isReactive(value) &&
+          !value?.__v_skip
+        ) {
+          diagnostics.PINIA_R1006({ key, id: $id })
+        }
+      }
+    }
+
+    assign(store, extensions)
   })
 
   if (

packages/pinia/src/storeToRefs.ts

@@ -9,7 +9,6 @@ import {
   ToRefs,
   WritableComputedRef,
 } from 'vue'
-import { diagnostics } from './diagnostics'
 import { StoreGetters, StoreState } from './store'
 import type {
   _ActionsTree,
@@ -110,17 +109,6 @@ export function storeToRefs<SS extends StoreGeneric>(
       refs[key] =
         // ---
         toRef(store, key)
-    } else if (
-      __DEV__ &&
-      // an object that isn't a ref/reactive and that the user didn't markRaw()
-      // is silently dropped: warn so they wrap it as state or markRaw it.
-      // `null` is included (typeof null === 'object') as it is the most common
-      // way to hit this. Primitives, functions (actions) and store API like
-      // `$id` are skipped on purpose.
-      typeof value === 'object' &&
-      !(value as any)?.__v_skip
-    ) {
-      diagnostics.PINIA_R1006({ key, id: store.$id })
     }
   }