-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix(types): Don't double UnwrapRef in setup stores #2771
Conversation
✅ Deploy Preview for pinia-official canceled.
|
✅ Deploy Preview for pinia-playground ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
expectType<{ aRef: Ref<string> }>(store.anotherShallowRef) | ||
expectType<{ aRef: Ref<string> }>(store.$state.anotherShallowRef) |
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 was passing even without my change because types are correctly set when using option stores
expectType<{ aRef: Ref<string> }>(setupStore.anotherShallowRef) | ||
expectType<{ aRef: Ref<string> }>(setupStore.$state.anotherShallowRef) |
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.
my change causes that these 2 assertions pass as well
@posva this is ready for review :) |
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v2 #2771 +/- ##
==========================================
- Coverage 92.73% 92.50% -0.23%
==========================================
Files 14 14
Lines 1335 1335
Branches 233 221 -12
==========================================
- Hits 1238 1235 -3
- Misses 97 100 +3 ☔ View full report in Codecov by Sentry. |
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.
Thanks!
FYI this breaks in ecosystem-ci with latest Vue: https://github.com/vuejs/ecosystem-ci/actions/runs/11050587708/job/30698660187 |
I see it when upgrading to latest vue. It seems to be related to the new syntax with getter and setter on Refs. I will have to check deeper and probably avoid the custom setter part for stores |
fixes #2770
Without this change for setup stores
UnwrapRef
is applied to state type twice.First time after defineStore infers the
SS
type parameter and passes it to_ExtractStateFromSetupStore
:pinia/packages/pinia/src/types.ts
Line 586 in 3c8782e
Then the result of
_ExtractStateFromSetupStore<SS>
is passed toStoreDefinition
interface, which passes it toStore
type, which doespinia/packages/pinia/src/types.ts
Line 470 in 3c8782e
First unwrap call reduces state type
Second unwrap call reduces it further producing type that doesn't match the runtime value