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.

Sign up for GitHub

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

Jump to bottom

Fix default namespace not working for fallback keys, fix types #470

Merged
merged 5 commits into from
Jan 26, 2021
Merged

Fix default namespace not working for fallback keys, fix types #470

merged 5 commits into from
Jan 26, 2021

Conversation

andrehsu
Copy link
Contributor

No description provided.

@andrehsu
Copy link
Contributor Author

@aralroca Is there some reason why all the types are defined in index.tsx, and not defined on the functions/classes themselves? I feel like a lot of the benefit of Typescript's static analysis is lost when the library uses so much type casting.

@andrehsu andrehsu changed the title Fix default namespace not working for fallback keys Fix default namespace not working for fallback keys, fix types Jan 26, 2021
@andrehsu
Copy link
Contributor Author

andrehsu commented Jan 26, 2021 •
edited
Loading

I took the liberty of replacing type casts with return types and type parameters where it makes sense, to make it more idiomatic Typescript.

aralroca
aralroca approved these changes Jan 26, 2021
View reviewed changes
Copy link
Owner

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

@andrehsu thanks a lot for your contribution! It's amazing! About the TypeScript types, they are in the index.ts maybe because of my lack of TypeScript experience. If you think it could be better, feel free to PR!

@aralroca aralroca added this to the 1.0.2 milestone Jan 26, 2021
@aralroca aralroca merged commit 3607d16 into aralroca:canary Jan 26, 2021
@aralroca
Copy link
Owner

@allcontributors please add @andrehsu for code

@allcontributors allcontributors bot mentioned this pull request Jan 26, 2021
Merged
@allcontributors
Copy link
Contributor

@aralroca

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

@aralroca
Copy link
Owner

@andrehsu I prepublished 1.0.2-canary.8 with your change if you want to start using it

@gurkerl83 gurkerl83 mentioned this pull request Jan 31, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aralroca aralroca aralroca approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.2
Development

Successfully merging this pull request may close these issues.
2 participants
@andrehsu @aralroca