vuejs / pinia

🍍 Intuitive, type safe, light and flexible Store for Vue using the composition api with DevTools support
https://pinia.vuejs.org
MIT License
12.91k stars 1.02k forks source link

fix(types): fix storeToRefs state return type (#2574) #2604

Closed nkeyy0 closed 5 months ago

nkeyy0 commented 6 months ago

Add new mapped type to keep original ref type in the storeToRefs return type (close #2574)

netlify[bot] commented 6 months ago

Deploy Preview for pinia-official canceled.

Name Link
Latest commit 50aa53b524041cc9dac3ece06c228e3f78a6bd22
Latest deploy log https://app.netlify.com/sites/pinia-official/deploys/660a9e899cf265000849d0d0
netlify[bot] commented 6 months ago

Deploy Preview for pinia-playground ready!

Name Link
Latest commit 50aa53b524041cc9dac3ece06c228e3f78a6bd22
Latest deploy log https://app.netlify.com/sites/pinia-playground/deploys/660a9e8936575d0008e0d7fb
Deploy Preview https://deploy-preview-2604--pinia-playground.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov-commenter commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.35%. Comparing base (f695b62) to head (1f9939c). Report is 20 commits behind head on v2.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## v2 #2604 +/- ## ========================================== - Coverage 95.42% 95.35% -0.08% ========================================== Files 11 11 Lines 2886 2904 +18 Branches 190 190 ========================================== + Hits 2754 2769 +15 - Misses 131 134 +3 Partials 1 1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nkeyy0 commented 5 months ago

Unfortunately, I won't have access to my laptop for the next 3-4 days and won't be able to rebase against v2 during this time. If it's urgent, feel free to handle it yourself. Otherwise, I can assist once I'm available. Let me know if there's anything else I can do

posva commented 5 months ago

It's not urgent, don't worry 🙂

nkeyy0 commented 5 months ago

Initially, I forgot to handle the case with actions in the store, so I made some updates to the type and added tests for the store that contained the actions.

nkeyy0 commented 5 months ago

Also, I've rebased my feature branch onto v2 and tested it locally. All good on my end. Let me know if you need anything else!

posva commented 5 months ago

Unfortunately, this wasn't fixing it; it set the returned type of storeToRefs to any, so I had to revert it. I should be able to give it more time in the following months or if any company really needs this, they can sponsor me and I will give it a higher priority

nkeyy0 commented 5 months ago

Unfortunately, this wasn't fixing it; it set the returned type of storeToRefs to any, so I had to revert it. I should be able to give it more time in the following months or if any company really needs this, they can sponsor me and I will give it a higher priority

To be honest it's a bit unexpected. Could you provide cases when the return type is set to any? Cause in type tests I didn't receive any return type. Maybe I just forgot to handle some specific case

posva commented 5 months ago

It was unexpected to me too. See https://github.com/vuejs/pinia/commit/5c1c3e1d519c3b14b0f42e680e03441a00c5e886 and the failed checks in other commits.

nkeyy0 commented 5 months ago

Interesting that the initial commit was successful, but once you removed the declare it started to fail. image

I've tested it locally and looks like this commit broke the pipeline. It seems like adding @internal to JSDoc somehow changes the build process and pinia.d.ts is not generated correctly and I'm getting errors in type tests too. Could you try to remove @internal and give it one more try?)

nkeyy0 commented 5 months ago

Yeah, looks like adding @internal to JSDoc prevents emitting a declaration of ToStateRefs type and that's why we get any as a result. https://www.typescriptlang.org/tsconfig/#stripInternal, so removing @internal should fix the issue

nkeyy0 commented 5 months ago

@posva Just tagging you as hope this can still make it to release