fix: never use global context on the server side #2983
Conversation
✅ Deploy Preview for pinia-official canceled.
|
packages/pinia/src/rootStore.ts
Outdated
| @@ -43,7 +43,7 @@ interface _SetActivePinia { | |||
| * Get the currently active pinia if there is any. | |||
| */ | |||
| export const getActivePinia = () => | |||
| (hasInjectionContext() && inject(piniaSymbol)) || activePinia | |||
| (hasInjectionContext() && inject(piniaSymbol)) || (import.meta.server ? throw new Error("Cannot get active pinia as it does not find context") : activePinia) | |||
There was a problem hiding this comment.
Thanks for the PR. This could be changed into a non breaking change by just showing an error and still returning the active pinia. The error should be in development only (like other warnings) and should not rely on import as it might not work in all scenarios
There was a problem hiding this comment.
I think failing fast is the better approach when the global activePinia is accessed on the server, because:
- failing immediately makes the problem obvious rather than silently continuing with potentially incorrect behavior.
- allowing it to continue by returning
activePiniacould lead to dangerous state sharing between requests, which is particularly problematic in server environments.
maybe making the message is more clear would help huge
There was a problem hiding this comment.
That would be a breaking change. Also, I do prefer the error message because it feels less frustrating to users
There was a problem hiding this comment.
I will leave the decision to you,
However, please keep in mind that the developers tend to ignore error messages, so they will make the same mistakes again and again, I would prefer it to fail that will indicate the mistake much faster.
|
@posva I removed throwing error, added |
when store is accessed and injection context is not found it falls back to global active pinia object,
although it is safe for client, on the server side there are could be many requests with their own context,
falling back to global state leads to race conditions and undefined behaviour,
this should be fixed once and for all!
it is probably a breaking change but a good one, I haven't tested this locally, I appreciate any help to improve this PR in order to make it good looking