Skip to content
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): fix storeToRefs state return type (#2574) #2604

Merged

Conversation

nkeyy0
Copy link
Contributor

@nkeyy0 nkeyy0 commented Mar 9, 2024

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

Copy link

netlify bot commented Mar 9, 2024

Deploy Preview for pinia-official canceled.

Name Link
🔨 Latest commit 50aa53b
🔍 Latest deploy log https://app.netlify.com/sites/pinia-official/deploys/660a9e899cf265000849d0d0

Copy link

netlify bot commented Mar 9, 2024

Deploy Preview for pinia-playground ready!

Name Link
🔨 Latest commit 50aa53b
🔍 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.

@nkeyy0 nkeyy0 changed the title fix(types): mapHelpers with getters types (#2574) fix(types): fix storeToRefs state return type (#2574) Mar 9, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

Additional details and impacted files
@@            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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Could you add a test for an options store using a special ref within the state() fn too?

@nkeyy0 nkeyy0 requested a review from posva March 29, 2024 13:14
Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I need to test locally with the latest dependencies. Or if you have the time could you rebase against v2 to get the latest deps?

@nkeyy0
Copy link
Contributor Author

nkeyy0 commented Mar 29, 2024

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

Copy link
Member

posva commented Mar 29, 2024

It's not urgent, don't worry 🙂

@nkeyy0 nkeyy0 requested a review from posva April 1, 2024 11:25
@nkeyy0
Copy link
Contributor Author

nkeyy0 commented Apr 1, 2024

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 nkeyy0 force-pushed the fix/infer-original-state-refs-in-store-to-refs branch from 9c5f7ec to 50aa53b Compare April 1, 2024 11:46
@nkeyy0
Copy link
Contributor Author

nkeyy0 commented Apr 1, 2024

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!

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@posva posva merged commit c8f727a into vuejs:v2 Apr 2, 2024
6 checks passed
@posva
Copy link
Member

posva commented Apr 4, 2024

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

posva added a commit that referenced this pull request Apr 4, 2024
@nkeyy0
Copy link
Contributor Author

nkeyy0 commented Apr 4, 2024

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
Copy link
Member

posva commented Apr 4, 2024

It was unexpected to me too. See 5c1c3e1 and the failed checks in other commits.

@nkeyy0
Copy link
Contributor Author

nkeyy0 commented Apr 4, 2024

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
Copy link
Contributor Author

nkeyy0 commented Apr 4, 2024

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
Copy link
Contributor Author

nkeyy0 commented Apr 5, 2024

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

posva added a commit that referenced this pull request Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

storeToRefs loses the original typings of refs/computeds in a setup store, "generalizes" them instead
3 participants