-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
docs(core): explain how to create dynamic store instances using defin… #3056
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
Conversation
✅ Deploy Preview for pinia-official ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughDocumentation additions explaining dynamic store IDs feature, introducing per-instance store creation using Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (2)
packages/docs/core-concepts/index.md (2)
175-188: Clarify the pattern's mechanics and add safety for undefinedid.The trailing
()invocation on line 188 could benefit from brief explanation—it converts the returned composable into a store instance. Additionally, sinceidis optional, stores could inadvertently share state when called without an argument (all resulting inBroadcastStore-undefined).Consider either requiring the
idparameter or providing a default:-export const useBroadcastStore = (id?: string) => - defineStore(`BroadcastStore-${id}`, { +export const useBroadcastStore = (id: string = Math.random().toString()) => + defineStore(`BroadcastStore-${id}`, {Alternatively, add a note cautioning against omitting
idif state isolation is critical.
175-204: Consider adding context on SSR compatibility and devtools behavior.Given the SSR warnings earlier in the documentation (line 79 and elsewhere), it would be helpful to note whether this dynamic store pattern works reliably with SSR and how it appears in devtools (e.g., do all instances of
BroadcastStore-aappear as a single store or separate entries?). This context would help developers decide when to use this pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/docs/core-concepts/index.md(1 hunks)
🔇 Additional comments (1)
packages/docs/core-concepts/index.md (1)
177-178: Clarify when state is shared vs. isolated across component instances.The claim "Each call creates an independent store" could mislead readers. If
useBroadcastStore('a')is called from multiple components, they all access the same store instance and share state. This is likely the intended behavior, but the docs should explicitly clarify whether isolation is per-call-site, per-component-instance, or per-ID.
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.
Believe this is a great addition. It's quite important but not generally documented. People might not know this is possible.
|
|
||
| ```ts | ||
| export const useBroadcastStore = (id?: string) => | ||
| defineStore(`BroadcastStore-${id}`, { |
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.
This is actually not good because it re defines the same store over and over and it's not expected behavior 😅
I see this behavior of trying to reuse a store a lot of times but it shouldn't be this way, it should have a collection within the store and then within a composable use that store and pass the other arguments
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.
I added a ticket to the roadmap to not forget about this, thanks for the PR thought! 🙏
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.
This is actually not good because it re defines the same store over and over and it's not expected behavior 😅
I see this behavior of trying to reuse a store a lot of times but it shouldn't be this way, it should have a collection within the store and then within a composable use that store and pass the other arguments
That's quite interesting, I was thinking I was being smart. Lol. What's the issue with this approach?
✨ Description
This PR adds a concise section demonstrating how to create dynamic Pinia store instances using unique IDs with
defineStore().It introduces a practical pattern like:
Each call to
useBroadcastStore(id)returns an independent store instance that shares the same structure but maintains its own state.Why
Example Use Cases
Summary
This addition enhances the Defining a Store guide by showing how to use
defineStore()dynamically to create multiple independent store instances with shared logic — a common need in real-world applications.Summary by CodeRabbit