-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: expose store options in PiniaCustomProperties for plugin access #3042
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
base: v3
Are you sure you want to change the base?
feat: expose store options in PiniaCustomProperties for plugin access #3042
Conversation
- Add StoreOptionsAccess utility type for accessing custom store options - Modify store creation to include _options property with store options - Export StoreOptionsAccess type from main module - Add comprehensive tests for plugin access to custom store options - Support both option stores and setup stores - Maintain backward compatibility with existing plugins Fixes vuejs#1247
✅ Deploy Preview for pinia-official canceled.
|
WalkthroughExposes per-store defineStore options at runtime via a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Pinia
participant Store
participant Plugin
App->>Pinia: createPinia()
App->>Pinia: pinia.use(Plugin)
App->>Pinia: defineStore(options) %% options include customOption / stores / debounce
App->>Store: useMainStore(pinia)
Note right of Store #D3E4CD: createSetupStore constructs instance\nand assigns _options = optionsForPlugin
Pinia-->>Plugin: initialize plugin with { store, options }
Plugin->>Store: read context.options / store._options
Plugin-->>App: expose computed getters based on options
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/pinia/src/store.ts (2)
471-489
: Store options should be non-reactive to avoid needless tracking
_options
currently references a reactive object. Mark it raw when storing on the reactivestore
to avoid proxying user options and functions.const store: Store<Id, S, G, A> = reactive( __DEV__ || (__USE_DEVTOOLS__ && IS_CLIENT) ? assign( { _hmrPayload, _customProperties: markRaw(new Set<string>()), // devtools custom properties - _options: optionsForPlugin, // store options for plugins + _options: markRaw(optionsForPlugin), // store options for plugins }, partialStore ) : assign( { - _options: optionsForPlugin, // store options for plugins + _options: markRaw(optionsForPlugin), // store options for plugins }, partialStore ) ) as unknown as Store<Id, S, G, A>
682-701
: Hide_options
from devtools/object enumerationInternal fields are made non-enumerable for devtools;
_options
should be included to prevent leaking internal shapes.- ;(['_p', '_hmrPayload', '_getters', '_customProperties'] as const).forEach( + ;(['_p', '_hmrPayload', '_getters', '_customProperties', '_options'] as const).forEach( (p) => { Object.defineProperty( store, p, assign({ value: store[p] }, nonEnumerable) ) } )
🧹 Nitpick comments (4)
packages/pinia/src/types.ts (1)
277-284
: Avoidany
on internal_options
Typing
_options
asany
leaks unsoundness into the public store type. Preferunknown
(safer), since plugins should access options viacontext.options
and type utilities, not through_options
.- _options?: any + _options?: unknownpackages/pinia/src/store.ts (1)
592-679
: HMR: keep_options
in syncDuring HMR,
_options
is not updated, so plugins readingstore._options
will see stale values in dev. Copy it fromnewStore
.// update the values used in devtools and to allow deleting new properties later on store._hmrPayload = newStore._hmrPayload store._getters = newStore._getters + // keep plugin options in sync during HMR + // @ts-expect-error: internal field + store._options = newStore._options store._hotUpdating = falsepackages/pinia/__tests__/storeOptionsAccess.spec.ts (2)
2-2
: ImportStoreOptionsAccess
for stronger runtime test typingsUse the new utility in the module augmentation to validate types here as well (mirrors the dts test).
-import { createPinia, defineStore, StoreDefinition } from '../src' +import { createPinia, defineStore, StoreDefinition, StoreOptionsAccess } from '../src'
14-18
: UseStoreOptionsAccess
in augmentation instead ofany
This exercise ensures the new utility works in real tests, not only in dts.
export interface PiniaCustomProperties<Id, S, G, A> { - readonly stores: any - readonly customOption: any - readonly debounce: any + readonly stores: StoreOptionsAccess<this, 'stores'> + readonly customOption: StoreOptionsAccess<this, 'customOption'> + readonly debounce: StoreOptionsAccess<this, 'debounce'> }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/pinia/__tests__/storeOptionsAccess.spec.ts
(1 hunks)packages/pinia/src/index.ts
(1 hunks)packages/pinia/src/store.ts
(1 hunks)packages/pinia/src/types.ts
(2 hunks)packages/pinia/test-dts/storeOptionsAccess.test-d.ts
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/pinia/src/types.ts (1)
packages/pinia/src/index.ts (2)
StoreOptionsAccess
(31-31)Store
(18-18)
packages/pinia/__tests__/storeOptionsAccess.spec.ts (2)
packages/pinia/src/types.ts (4)
DefineStoreOptionsBase
(635-635)Store
(473-485)StoreDefinition
(502-527)PiniaCustomProperties
(532-537)packages/pinia/src/store.ts (1)
defineStore
(844-939)
packages/pinia/test-dts/storeOptionsAccess.test-d.ts (2)
packages/pinia/__tests__/storeOptionsAccess.spec.ts (2)
DefineStoreOptionsBase
(8-12)PiniaCustomProperties
(14-18)packages/pinia/src/types.ts (5)
DefineStoreOptionsBase
(635-635)Store
(473-485)StoreDefinition
(502-527)PiniaCustomProperties
(532-537)StoreOptionsAccess
(556-556)
🔇 Additional comments (2)
packages/pinia/src/index.ts (1)
31-31
: LGTM: new public type exportRe-exporting
StoreOptionsAccess
from./types
looks correct and non-breaking.packages/pinia/test-dts/storeOptionsAccess.test-d.ts (1)
1-24
: LGTM: dts coverage is solidAugmentation and
StoreOptionsAccess
usage validate the intended typing surface.
/** | ||
* Utility type to access store options within PiniaCustomProperties. | ||
* This allows plugins to access custom options defined in DefineStoreOptionsBase. | ||
* | ||
* @example | ||
* ```ts | ||
* declare module 'pinia' { | ||
* export interface DefineStoreOptionsBase<S, Store> { | ||
* stores?: Record<string, StoreDefinition>; | ||
* } | ||
* | ||
* export interface PiniaCustomProperties<Id, S, G, A> { | ||
* readonly stores: any; // Use any for now, will be properly typed by plugins | ||
* } | ||
* } | ||
* ``` | ||
*/ | ||
export type StoreOptionsAccess<Store, Key extends keyof any> = any | ||
|
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.
Implement StoreOptionsAccess
to actually resolve plugin-augmented option types
Right now it’s any
, defeating the PR goal. Make it map a Store instance to its DefineStoreOptionsBase
augmentation and pick Key
, falling back to undefined
when absent.
-export type StoreOptionsAccess<Store, Key extends keyof any> = any
+/**
+ * Resolves the type of a custom option declared via `DefineStoreOptionsBase`
+ * for the given Store instance type and option `Key`.
+ * Returns `undefined` if the option is not declared.
+ */
+export type StoreOptionsAccess<
+ This,
+ Key extends keyof any,
+ Fallback = undefined,
+> =
+ This extends Store<infer Id, infer S, infer G, infer A>
+ ? DefineStoreOptionsBase<S, Store<Id, S, G, A>> extends infer O
+ ? O extends Record<PropertyKey, unknown>
+ ? Key extends keyof O
+ ? O[Key]
+ : Fallback
+ : Fallback
+ : Fallback
+ : Fallback
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
/** | |
* Utility type to access store options within PiniaCustomProperties. | |
* This allows plugins to access custom options defined in DefineStoreOptionsBase. | |
* | |
* @example | |
* ```ts | |
* declare module 'pinia' { | |
* export interface DefineStoreOptionsBase<S, Store> { | |
* stores?: Record<string, StoreDefinition>; | |
* } | |
* | |
* export interface PiniaCustomProperties<Id, S, G, A> { | |
* readonly stores: any; // Use any for now, will be properly typed by plugins | |
* } | |
* } | |
* ``` | |
*/ | |
export type StoreOptionsAccess<Store, Key extends keyof any> = any | |
/** | |
* Utility type to access store options within PiniaCustomProperties. | |
* This allows plugins to access custom options defined in DefineStoreOptionsBase. | |
* | |
* @example | |
* |
🤖 Prompt for AI Agents
In packages/pinia/src/types.ts around lines 539 to 557, StoreOptionsAccess is
currently typed as any; replace it with a conditional/lookup type that, given a
Store instance type, resolves the corresponding augmented DefineStoreOptionsBase
for that store and picks the property Key (returning that property type or
undefined if missing). Implement this using TypeScript conditional/lookup types
that extract the appropriate DefineStoreOptionsBase augmentation for the
provided Store type (e.g., by inferring the store identifier or matching the
Store instance type to the augmented interface) and return the property at Key
or undefined as a fallback.
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
…operties` Docstrings generation was requested by @doubledare704. * #3042 (comment) The following files were modified: * `packages/pinia/src/store.ts`
Note Generated docstrings for this pull request at #3044 |
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/pinia/src/store.ts (3)
487-505
: Make_options
non-reactive to avoid devtools/serialization churn.Right now
_options
becomes reactive (proxied) and enumerable. This can:
- Surface large, non-serializable trees/functions in devtools and snapshots.
- Add needless proxy work.
- Allow accidental mutation by plugins/userland.
Minimal safe change: mark it raw.
? assign( { _hmrPayload, _customProperties: markRaw(new Set<string>()), // devtools custom properties - _options: optionsForPlugin, // store options for plugins + _options: markRaw(optionsForPlugin), // store options for plugins }, partialStore ) : assign( { - _options: optionsForPlugin, // store options for plugins + _options: markRaw(optionsForPlugin), // store options for plugins }, partialStore )If you want the stronger guarantee (recommended): define
_options
as non-enumerable and readonly right after store creation instead of including it in theassign()
object.// place right after the reactive(store) creation, once optionsForPlugin is fully prepared Object.defineProperty(store, '_options', { value: markRaw(optionsForPlugin), enumerable: false, configurable: true, // allow HMR to redefine writable: false, })
706-716
: Hide_options
from devtools property listing.For consistency with other internals and to avoid noisy/unsafe serialization, include
_options
in the non-enumerable set.- ;(['_p', '_hmrPayload', '_getters', '_customProperties'] as const).forEach( + ;(['_p', '_hmrPayload', '_getters', '_customProperties', '_options'] as const).forEach(
609-695
: Sync _options during HMRHMR copies actions/getters/state but not store._options, so plugins reading per-store options after HMR will see stale values — update _options near the end of store._hotUpdate (before updating _hmrPayload/_getters):
// sync plugin options if ('_options' in newStore) { Object.defineProperty(store, '_options', { value: newStore._options, enumerable: false, configurable: true, writable: false, }) }File: packages/pinia/src/store.ts — inside store._hotUpdate (approx. lines 609–695).
🧹 Nitpick comments (1)
packages/pinia/src/store.ts (1)
216-231
: Docblock LGTM (tiny nits optional).Reads clearly and matches the function behavior. If you want, tighten wording: “Returns a reactive store instance that exposes …” instead of “The reactive Store instance is exposing …”.
- Update docstring for `createSetupStore` return type for clarity - Mark `_options` as raw to prevent reactivity issues - Add syncing of `_options` property during HMR - Ensure `_options` is non-enumerable and consistent in internal properties
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.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/pinia/src/store.ts (4)
216-231
: Good docblock; consider markingisOptionsStore
as internal.To avoid implying public support, add an
@internal
note forisOptionsStore
in the TSDoc.
493-493
: Avoid enumerating_options
on the public store in non-devtools builds.Inline
assign(..., { _options: ... })
makes_options
enumerable (SSR/plugins iterating keys will see it). Prefer defining it as non-enumerable right after store creation.Apply this diff to stop setting
_options
here:- _customProperties: markRaw(new Set<string>()), // devtools custom properties - _options: markRaw(optionsForPlugin), // store options for plugins + _customProperties: markRaw(new Set<string>()), // devtools custom propertiesThen, right after the reactive store is created (below Line 505), add:
Object.defineProperty(store as any, '_options', { value: markRaw(optionsForPlugin), enumerable: false, configurable: true, writable: true, // keep parity with other internals; see devtools block })
499-504
: Same enumerability concern for non-devtools path.Make
_options
non-enumerable at definition time for consistency with the devtools path and to avoid leaking into key iterations.Apply this diff:
- : assign( - { - _options: markRaw(optionsForPlugin), // store options for plugins - }, - partialStore - ) + : assign({}, partialStore)(Then keep the single
Object.defineProperty
addition suggested above to define_options
once.)
691-699
: HMR redefines_options
with different writability/enumerability; preserve prior descriptor.Currently HMR sets
writable: false
andenumerable: false
, which may diverge from initial flags and surprise plugins. Reuse the existing descriptor’s flags.Apply this diff:
- if ('_options' in newStore) { - Object.defineProperty(store, '_options', { - value: newStore._options, - enumerable: false, - configurable: true, - writable: false, - }) - } + if ('_options' in newStore) { + const prev = Object.getOwnPropertyDescriptor(store, '_options') + Object.defineProperty(store, '_options', { + value: newStore._options, + enumerable: prev?.enumerable ?? false, + configurable: true, + writable: prev?.writable ?? true, + }) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/pinia/src/store.ts
(4 hunks)
🔇 Additional comments (1)
packages/pinia/src/store.ts (1)
716-731
: Hiding_options
from devtools is correct — verify locally.packages/pinia/src/store.ts (≈lines 700–740) shows '_options' included in the non-enumerable list; keeping it non-enumerable is appropriate. Sandbox could not run workspace tests (pnpm test failed due to missing dev tooling/node_modules). Run locally and confirm no regressions and no unintended writes to _options:
pnpm -w install
pnpm -w test
pnpm -w -r test-dts
rg -n --hidden -S '._options\s*=' -g '!/node_modules/' -C2
Fixes #1247
Summary by CodeRabbit
New Features
Tests