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

Improve Typescript types and generics #22

Merged
merged 6 commits into from
May 30, 2019

Conversation

Lynncubus
Copy link
Contributor

Fixes #21

Exports a few types for usage outside of the module and adds default types for generics.

@drcmda
Copy link
Member

drcmda commented May 24, 2019

Looks good to me, we need these types as well at work, duplicated them for now. @JeremyRH how do you feel? You still wanted to remove the internal types, like StateSelector, is this correct?

@JeremyRH
Copy link
Contributor

The reason for not wanting to expose all the types is mostly for type names. If we change a name, it's a breaking change. I do see the value for not duplicating types though so I'm thinking we should expose all the types.

I'll test out these changes today and try to get this merged. Thanks @Luxizzle!

@dm385
Copy link

dm385 commented May 25, 2019

When using "zustand" in one of our projects, I had defined the types UseStore and StoreApi in globals.d.ts myself. Now I came across this pull request. If I now compile the project with Luxizzle's solution directly with tsc, I get some compiler errors. These can also be reproduced in this repo when compiling the file test.tsx. For this reason, I have adapted the definitions in my own branch and adapted the configuration so that it can be compiled directly with tsc (yarn build:tsc).

You can have a look at it if you like:
improve-typescript

Please correct me if I am wrong.

@JeremyRH
Copy link
Contributor

JeremyRH commented May 25, 2019

@dm385 You're right, the tests were failing with Luxizzle's changes. I asked them to create a PR so we could work on fixing the issues together. I have a version of their changes working without changing the tests now.

And thank you for the fixes on your fork. I'll see if there's anything there that I can add to this.

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@Lynncubus
Copy link
Contributor Author

I updated everything with the proposed changes you gave. Thanks for all the explanations by the way, they really helped!

Copy link
Contributor

@JeremyRH JeremyRH left a comment

Choose a reason for hiding this comment

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

Thanks!

@JeremyRH JeremyRH merged commit 9d6be42 into pmndrs:master May 30, 2019
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.

Improve Typescript support and only make the first generic type required.
4 participants