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 for optional stores #94

Merged
merged 3 commits into from
Sep 1, 2022

Conversation

filoozom
Copy link
Contributor

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 and withStoreType 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:

type User = {
  username: string;
  avatar: string | undefined;
}

type Store = {
  user: User | undefined
}

const { useStore, getStore, withStore, setStore } = createStore<Store>()

The following function calls are marked as invalid by TypeScript, but do work through the Proxy:

setStore.user.username() // Property 'username' does not exist on type 'setter<User | undefined>'.
getStore.user.username() // Property 'username' does not exist on type 'HookDry<User | undefined>'.
useStore.user.username() // Property 'username' does not exist on type 'Hook<User | undefined>'.
withStore.user.username() // Property 'username' does not exist on type 'HocFunc<Store, ComponentClass<{}, any>>'.

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 and useStore for example. This PR changes those as such:

const [username, setUsername] = getStore.user.username()

typeof username = string;
typeof setUsername = setter<string>
typeof getStore.user = useStoreType<User> & HookDry<User | undefined>

I think at least username should be string? instead of string or getStore.user should be Optional<useStoreType<User> & HookDry<User | undefined>>. The latter makes less sense to me, as technically the code runs and doesn't throw a Cannot read properties of undefined (reading 'user') error, but the fact that username might be undefined as long a one of its parent keys is undefined should be clear.

@aralroca
Copy link
Collaborator

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.

@filoozom
Copy link
Contributor Author

Hi @aralroca, thank you for your fast answer! I'm afraid it does still happen with the latest version.

@aralroca
Copy link
Collaborator

aralroca commented Aug 31, 2022

@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!

Copy link
Collaborator

@aralroca aralroca left a 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!

@filoozom filoozom force-pushed the fix/types-for-optional-stores branch from d3993ec to ff2ac3f Compare September 1, 2022 08:00
@aralroca aralroca merged commit cdc6ea6 into teafuljs:master Sep 1, 2022
@aralroca
Copy link
Collaborator

aralroca commented Sep 1, 2022

@all-contributors please add @filoozom for code

@allcontributors
Copy link
Contributor

@aralroca

I've put up a pull request to add @filoozom! 🎉

@aralroca
Copy link
Collaborator

aralroca commented Sep 1, 2022

@filoozom I released 0.11 with your change and more TypeScript changes! 👍

@filoozom filoozom deleted the fix/types-for-optional-stores branch September 1, 2022 13:43
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.

None yet

2 participants