-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Change the API #18
Comments
This looks good to me! Will |
I think that seconds would be the best time resolution as reading times generally vary from under 1 minute to several minutes. It's easily convertible to |
Sounds good! |
I hope you don't mind me pulling you here @Josh-Cena but I'm curious about your opinion on this. As a user, would you like an API similar to the one proposed here for |
No strong feelings on this, both seem acceptable to me. But considering the utility nature of this lib, I would prefer a simpler API that returns more data (i.e. the current API) so the user has less to maintain on their side. Getting rid of the |
Got it. I agree that the So the new result would just be: type Result = {
minutes: minutes,
time: time,
words: words
} I still kind of like the idea of decoupling word counting from reading time computation in case users want to customize things. It's pretty cheap to achieve so why not 🤷 I think we can do that while not breaking the current API: type countWords = (text: string, options?: { bound?: string => boolean }): number
type readingTimeWith = (wordCount: number, options?: { wordsPerMinute?: number }): Result
type readingTime = (text: string, options?: { wordsPerMinute?: number }): Result @Josh-Cena What do you think? |
LGTM, we should allow people to control this process more granularly, while one single convenient API would still be good for normal users👍 |
Another thing I noticed when I'm refactoring the test suite: the |
Currently
readingTime
returns an object of the form:That's too much and it's not easily composable. Let's decouple things and expose this API:
The text was updated successfully, but these errors were encountered: