Remove moment as a dependency #11
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3imed-jaberi
requested changes
Oct 11, 2024
There was a problem hiding this comment.
@Souler Great job! I had a plan to remove 'moment' or keep it as peer deps. a while ago.
Here you can find a few refinement suggestions, could you please apply them and update all tests. once you do that, ping me to move forward!
|
@3imed-jaberi I went ahead and addressed the naming suggestion and the unnecesary variable on However I find myself arguing against the other two proposed changes (the object-map for months, and the array for joining the time). Happy to discuss them further so we can find an agreement. |
3imed-jaberi
requested changes
Oct 12, 2024
3imed-jaberi
approved these changes
Oct 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For as simple as this
koa-accesslogis; bringing moment into the dependency tree just to format a date felt a bit overkill.This PR replaces moment with a local minimal implementation only with en support.
Why not using Intl?
Intl availability on runtime depends on how the node binary was built and how the process running was started (you can read more here). Since I did not want to couple the library to a specific icu setup; I decided to implement the minimal required code for the date formatting needs of the library.
## Would this be a breaking change?
To be honest: I'm not 100% sure. I have tried to keep the implementation as close as possible to what we had; but this PR would change how the short month is rendered for non-en configured systems. Not a big deal; but worht considering when decieding.
It is also worth noting this PR is indepentent from #10 but they would create a conflict with eachother because of the changes in the
package-lock.jsonin the other PR.