-
-
Notifications
You must be signed in to change notification settings - Fork 25
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 for optional stores #94
fix: types for optional stores #94
Conversation
Thank you for your contribution @filoozom. Before reviewing the PR, would you confirm if the same issue is happening in 0.11.0-canary.2 prerelease? Thank you! We were working to migrate the project to TypeScript in this PR #68 (needs to be merged yet, but we can merge it at any time so we can work better on the new types), and we take the opportunity to fix some issues with types. After this change, the tests check the types, probably we can add this failing case. |
Hi @aralroca, thank you for your fast answer! I'm afraid it does still happen with the latest version. |
@filoozom I just merged the PR migrating to TypeScript and will do the 0.11 release once what you are talking about works. In the end, whether or not it works for you with the PR I've merged now, it would be nice to pass the new tests with this bug in mind. Sorry if that means you have to fix some conflicts and add the test. Thank you very much for your contribution! |
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.
@filoozom I have been reviewing the changes you have made and I see good to apply them. I'm fine to merge this PR when the conflicts will be fixed and the tests adapted. Thanks for your effort!
d3993ec
to
ff2ac3f
Compare
@all-contributors please add @filoozom for code |
I've put up a pull request to add @filoozom! 🎉 |
@filoozom I released 0.11 with your change and more TypeScript changes! 👍 |
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[x] Bug fix
[ ] New feature
[ ] Other, please explain:
What changes did you make? (Give an overview)
Changed the types of
getStoreType
,useStoreType
,setStoreType
andwithStoreType
to accept stores where some keys are optional.Which issue (if any) does this pull request address?
I didn't open an issue beforehand, but I'll try to explain the issue. Imagine we have the following store:
The following function calls are marked as invalid by TypeScript, but do work through the Proxy:
Is there anything you'd like reviewers to focus on?
I'm not sure this is the best way to do things, and I'm also not too sure how you'd prefer your types to be for
getStore
,withStore
anduseStore
for example. This PR changes those as such:I think at least
username
should bestring?
instead ofstring
orgetStore.user
should beOptional<useStoreType<User> & HookDry<User | undefined>>
. The latter makes less sense to me, as technically the code runs and doesn't throw aCannot read properties of undefined (reading 'user')
error, but the fact thatusername
might be undefined as long a one of its parent keys isundefined
should be clear.