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

refactor: major API changes #42

Merged
merged 7 commits into from
Oct 4, 2021
Merged

refactor: major API changes #42

merged 7 commits into from
Oct 4, 2021

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented Sep 13, 2021

Resolve #18.

Breaking changes

  • The readingTime now only returns time (milliseconds) and minutes—both are integers. No strings or decimals are returned. You can always convert them to representations you need.
    • Why? Removal of text improves ease of i18n as we are no longer biased towards English. minutes used to return a decimal which is trivially time / 60000 but with a lot of floating point funkiness.
  • The ReadingTimeStream now only stores word count as its state—it is the equivalent of countWords(), not the bulk of readingTime(). Users still need to run readingTimeWithCount() to get the result.
    • Why? To improve composability, because a lot of the implementation details that go on under the hood are not controllable to the user, so a granular API is more desirable.

I may be cutting too many features here. Open to discussion.

Features

New functions countWords and readingTimeWithCount. Right now WordCountResults = { total: number }, but in the future we can have whitespace, punctuation, CJK, words, etc., each with its own wordPerMinute setting when passed to readingTimeWithCount!

@Josh-Cena Josh-Cena requested a review from ngryman September 14, 2021 03:08
Copy link
Owner

@ngryman ngryman left a comment

Choose a reason for hiding this comment

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

Hey @Josh-Cena, thanks a bunch for the PR. I like it 🤘

It mostly LGTM but since we're changing the API, I would go a bit further (cf. ReadingTimeResults) and I would change a few minor things.

ReadingTimeResults

I would even go further. I think we should just return seconds and that's it. Milliseconds don't make sense here and minutes can be derived easily as you mentioned.

So that would give us:

type ReadingTimeResults = {
  time: number;
  words: number;
};

Simple.

Minor changes

  1. ReadingTimeResultsReadingTimeResult since we only returning one result.
  2. wordCountcoundWord since it's a function, I prefer if it starts with a verb.
  3. Unless you were thinking about adding more properties to WordCountStats in the future, I prefer if we keep things simple and just return the word count.
  4. I don't have a super-strong opinion on this, but I generally prefer to use type for concrete types (ie. POJOs) and interface for abstract types (ie. to be inherited by a class).

PS: Could you add yourself to the contributors' section ❤️

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Sep 15, 2021

I kept the minutes field because our chief purpose is still to provide a "medium's like reading-time", and I don't want users to invest significantly in making that happen, even if it's simply Math.round(time / 60)🤪 Getting rid of the "1 min read" is already introducing one layer of indirectness, but I think that's worth it for the sake of i18n

Unless you were thinking about adding more properties to WordCountStats in the future,

I am😉 On my plans are whitespace, punctuation, CJK, words, etc., each with its own wordPerMinute setting when passed to readingTimeWithCount. For example Chinese speakers read more CJK characters than English speakers read Latin words, so there should be a nuance

Apart from those, great suggestions, will address those

Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@Josh-Cena Josh-Cena requested a review from ngryman September 16, 2021 14:52
@Josh-Cena
Copy link
Collaborator Author

Ping for @ngryman if we can settle on the API and merge this, we can release 2.0.0 because this is the last milestone

@ngryman ngryman merged commit a14a217 into ngryman:master Oct 4, 2021
@ngryman
Copy link
Owner

ngryman commented Oct 4, 2021

Hey @Josh-Cena, sorry again for the delay. I'll be more responsive now I'm finally home 🙂

@Josh-Cena
Copy link
Collaborator Author

No problem😄 Shall we release 2.0.0? Is there any other milestone on your mind?

@ngryman
Copy link
Owner

ngryman commented Oct 12, 2021

Shall we release 2.0.0?

Yes! Out of precaution, I published it as a 2.0.0-0 pre-release with a beta tag.
Here's the GH release: https://github.com/ngryman/reading-time/releases/tag/v2.0.0-0.

That will give us the time to potentially tweak things before the final 2.0.0 release.

Thanks for all the awesome work you did on this and sorry again for my lack of availability 🙏

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.

Change the API
2 participants