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

feat: Adding TS typings to library #22

Merged
merged 7 commits into from
Aug 22, 2020
Merged

feat: Adding TS typings to library #22

merged 7 commits into from
Aug 22, 2020

Conversation

rschristian
Copy link
Contributor

Without typings TS users are forced to declare the module as any, basically, losing all advantages of Typescript. Adding typings will vastly improve their experience and it will also improve the experience of JS users. IDEs with their intellisense will use these typings to help improve auto-complete and the like.

What I did was add types for all listed models. Let me know if I've missed anything or incorrectly typed something.

This might not be the most elegant solution, but I believe short of adding Typescript to the project this is the best solution.

@omgovich omgovich self-requested a review August 21, 2020 21:54
@omgovich
Copy link
Owner

omgovich commented Aug 21, 2020

@ryanchristian4427 That is so good! Helps to improve DX a lot!

Actually, I think about adding TS types every day, but I don't have much experience in TS, so you saved me a lot of time ❤️ Thank you!

Rewriting of the whole library to TS is in my plans (should make the library more stable), but I don't know when I'll be able to do that, and also I'm afraid that it might affect the bundle size. So your solution is perfect for now 🍻

@rschristian
Copy link
Contributor Author

I don't think it would increase the bundle size any considerable amount, if any. If I can find the time later I'll try to rewrite one of the models in TS and compare the outputs. Should be the same.

If size were to increase it would be with some of the stricter type checks, though one could say these checks, while increasing size, also increase correctness and reduce bugs. Haven't actually taken a look through (much of) the source though, so we'll see.

@rschristian
Copy link
Contributor Author

rschristian commented Aug 22, 2020

Having had more time to look through things, I'm guessing a switch to full TS would require a sizeable re-evaluation of the code. There's just no way to just add types to ColorPicker without changes as it stands.

It is late, forgive me, no re-evaluation needed. Got mixed up between some files/definitions.

@rschristian
Copy link
Contributor Author

That being said, a quick and messy TS setup for just the default package export (hex string) sees ~40 byte increase across the board. This is just the first pass, and there are added type checks to avoid accessing null/undefined, so not bad.

@omgovich
Copy link
Owner

Looks and works well! Thank you!

@omgovich omgovich merged commit 09c57c9 into omgovich:master Aug 22, 2020
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.

2 participants