fix: throw when a store is used outside of a Nuxt-aware context. #2857
Conversation
✅ Deploy Preview for pinia-playground canceled.
|
✅ Deploy Preview for pinia-official canceled.
|
"correctly" no. The purpose of this PR is to ease the detection of incorrect usage. In the current version bad usage of a store after a await will work without warning in low-traffic environments. But, in cases of simultaneous requests, there is a possibility that pinia uses the pinia instance from another request. I can provide a reproduction if needed, but this is already a documented case:
For context: We recently found out that we had cross-request pollution caused by bad use of Pinia in Nuxt. With our large codebase, it was tough to track down every instance of the issue. So, we ended up creating a custom version of (I'll update the PR description with those infos.) |
pguilbert
changed the title
|
Thanks for the great description of the problem! I agree it's a nice addition |
commit: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2 #2857 +/- ##
==========================================
- Coverage 88.75% 87.96% -0.79%
==========================================
Files 19 19
Lines 1449 1462 +13
Branches 226 226
==========================================
Hits 1286 1286
- Misses 162 175 +13
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Nice! I'll complete the typing and refine the test to move this PR out of draft. |
pguilbert
force-pushed
the
error-usage-after-await
branch
from
2a0c9ed to
d357d58
Compare
|
@posva what do you think of this version? Could we merge this? 🙏 |
| export const usePinia = () => useNuxtApp().$pinia | ||
| export const usePinia = () => useNuxtApp().$pinia as Pinia | undefined | ||
|
|
||
| export const defineStore: typeof _defineStore = ( |
There was a problem hiding this comment.
Let's make the error dev only, it shouldn't affect the final bundle. This should be possible by using process.env.NODE_ENV === 'production' ? _defineStore : ... or a similar guard
There was a problem hiding this comment.
Sorry it took so long; it's done now!
pguilbert
force-pushed
the
error-usage-after-await
branch
from
d357d58 to
1f3d17a
Compare
|
Thanks for this @pguilbert ! I think this is a great addition to Pinia. Is this planned to merge any time soon? 😊 @posva |
Prefer the Nuxt Pinia instance over the global active Pinia instance. Since the Nuxt Pinia instance is discarded after each request, it ensures that we can't accidentally use one from another request. Additionally, `usePinia` will throw an error when used outside of a Nuxt-aware context. The error is as follows in dev : > [nuxt] A composable that requires access to the Nuxt instance was called outside of a plugin, Nuxt hook, Nuxt middleware, or Vue setup function.
posva
force-pushed
the
error-usage-after-await
branch
from
5cf66cf to
05d6bf7
Compare
WalkthroughImplements SSR-safe store handling by wrapping Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant defineStore as SSR defineStore
participant Pinia
alt Non-Production/Test Environment
Client->>Server: Request store instance
Server->>defineStore: useStore() called
defineStore->>defineStore: Check if server context
defineStore->>Pinia: Defer to usePinia()
Pinia-->>defineStore: Return Pinia instance
defineStore->>defineStore: Proxy store creation
defineStore-->>Server: Return store via wrapper
else Production Environment
Client->>defineStore: useStore() called
defineStore->>Pinia: Direct _defineStore call
Pinia-->>defineStore: Return store
defineStore-->>Client: Return store instance
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used🧬 Code graph analysis (2)packages/nuxt/global.d.ts (1)
packages/nuxt/src/runtime/composables.ts (1)
🔇 Additional comments (9)
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 |
| @@ -0,0 +1,2 @@ | |||
| // Global compile-time constants | |||
| declare var __TEST__: boolean | |||
There was a problem hiding this comment.
Any way to get rid of this? I don't think we should need this
| : ((( | ||
| ...args: Parameters<typeof _defineStore> | ||
| ): ReturnType<typeof _defineStore> => { | ||
| if (!import.meta.server) { |
There was a problem hiding this comment.
This condition can be probably be moved up so we simply return _defineStore too
3 participants
Context
The Pinia documentation states the following:
We recently discovered that we had cross-request pollution caused by bad use of Pinia stores after an await in Nuxt. With a large codebase, it was tough to track down every instance of the issue. So, we ended up creating a custom version of
defineStore, similar to the one in this PR, to help prevent developers from misusing Pinia stores.I think it would be useful if this could be implemented directly in the Pinia Nuxt module.
Description of the changes
The Nuxt Pinia module does the following for each request:
The module also provides a helper
usePinia()that retrieves the Pinia instance from the Nuxt context and not from the Pinia global variable. Conveniently, this helper will throw an error if used after anawait(outside of a Nuxt-aware context)!This PR ensures that on the server we always use
usePinia()to get the current Pinia instance. By doing souseStorewill systematically throw if used after an await on the server (similarly to theuseNuxtAppcomposable)Breaking change
Server-side, using a store outside a Nuxt-aware context will now result in an error. Previously, it would continue without error, but sometimes, the wrong Pinia instance was used.
Error in dev:
Error in prod:
Summary by CodeRabbit
New Features
Documentation
Tests