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(twas): refracto to use intl api #974

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AugustinMauroy
Copy link
Contributor

@AugustinMauroy AugustinMauroy commented Feb 22, 2025

Use intl api instead of dep

Copy link
Contributor

@EGAMAGZ EGAMAGZ left a comment

Choose a reason for hiding this comment

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

I consider that it is better to keep the current library, since it has been in use for a longer time, has been extensively tested and, therefore, is more stable and reliable.

On the other hand, the library you suggest you just created it recently, exactly one hour ago, which implies that it has not yet been sufficiently evaluated in different scenarios. Also, since Deno is compatible with NPM, it is not strictly necessary to migrate to a library published in JSR unless there is a compelling reason, such as a deprecation or a significant advantage.

@AugustinMauroy
Copy link
Contributor Author

AugustinMauroy commented Feb 22, 2025

has been extensively tested

That's not right my lib have more test case and 100% test coverage. I'm working on CI that test the package on deno and bun

On the other hand, the library you suggest you just created it recently, exactly one hour ago, which implies that it has not yet been sufficiently evaluated in different scenarios. Also, since Deno is compatible with NPM, it is not strictly necessary to migrate to a library published in JSR unless there is a compelling reason, such as a deprecation or a significant advantage.

I agree with you on much of it. But don't forget that SJR's source code represents the entire project. And if it's too dependent on NPM, it's not very serious: a registry that needs another registry to function is “the snake that dies on its own tail”.

@EGAMAGZ
Copy link
Contributor

EGAMAGZ commented Feb 22, 2025

After reviewing the original package alongside your proposed implementation, I've noticed that the way time interval and time difference calculations are handled is very similar to the code found in this repository (the current package used), which is licensed under MIT. The logic and structure are nearly identical, and they even share the same naming conventions and package name—aside from a few added configuration and test files.

Could you please confirm whether both codebases have been carefully compared for any discrepancies, and that the proper MIT license attribution is maintained in both cases? This similarity reinforces my earlier point that the changes may not provide a significant improvement or benefit overall compared with current package.

@AugustinMauroy
Copy link
Contributor Author

Could you please confirm whether both codebases have been carefully compared for any discrepancies, and that the proper MIT license attribution is maintained in both cases? This similarity reinforces my earlier point that the changes may not provide a significant improvement or benefit overall compared with current package.

That just a rewrite in typescript and "modernize".
According to my research, this should not violate the MIT license.

@crowlKats
Copy link
Collaborator

I would actually be in favour of not using a dependency, and instead use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DurationFormat (or Duration from temporal)

@AugustinMauroy
Copy link
Contributor Author

AugustinMauroy commented Mar 11, 2025

I would actually be in favour of not using a dependency, and instead use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DurationFormat (or Duration from temporal)

I'll try to see how to use Intl

@AugustinMauroy AugustinMauroy changed the title dep(twas): use JSR dep(twas): use intl api Mar 11, 2025
@AugustinMauroy AugustinMauroy changed the title dep(twas): use intl api feat(twas): refracto to use intl api Mar 11, 2025
),
hours: Math.floor((diff % (1000 * 60 * 60 * 24)) / (1000 * 60 * 60)),
minutes: Math.floor((diff % (1000 * 60 * 60)) / (1000 * 60)),
seconds: Math.floor((diff % (1000 * 60)) / 1000),
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't think going into seconds is useful

// Force english because JSR is an English-only project
// @ts-ignore - TS doesn't know about this API yet
const formatter = new Intl.DurationFormat("en", { style: "long" });
return formatter.format(duration);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return formatter.format(duration);
return formatter.format(duration) + " ago";

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.

3 participants