Skip to content

Conversation

dougg0k
Copy link

@dougg0k dougg0k commented Jul 4, 2025

Edit:

All tests passing. Ready for review.

I suggest reading the whole description.


My other PR that need merging, it's a simple one. #2696 but unrelated to this one.


Hi,

I've refactored the code to have a single point encapsulating date functionality, that should make the code cleaner. It was a mess before, with a bunch of repetitions spread, making it harder to maintain or erven replace the implementation if that were ever needed.

https://day.js.org/

Momentjs is known to be a buggy library due to being mutable and with it's side effects. No point in supporting legacy code just because someone might want. But regardless, dayjs were designed as straight replacement.

They themselves recommend another solution https://github.com/arshaw/xdate/blob/master/README.md but thats 20kb, dayjs is only 2-3kb.


Here is a comparison between the previous and the new implementation.

XDate

  • Lightweight date library with a simple API.
  • Focuses on parsing, formatting, and manipulating dates.
  • Supports basic operations: add/subtract, set/get parts of a date, formatting.
  • No built-in support for time zones or locales.
  • No longer actively maintained (last update was several years ago).
  • Smaller ecosystem and less community support.
  • API is very similar to the native JS Date object but adds chainability and better parsing.

Day.js

  • Modern, fast, and minimalist date library (~2KB gzipped).
  • API is almost a drop-in replacement for Moment.js (intentionally designed this way).
  • Extensive plugin ecosystem (time zones, relative time, advanced formatting, custom parsing, durations, etc.).
  • Actively maintained and widely used.
  • Immutable date objects (operations return new instances, not mutate existing ones).
  • Supports internationalization (i18n) with locales.
  • Smaller bundle size than Moment.js, and typically more feature-rich than XDate.

Summary Table

Feature XDate Day.js
Maintenance Inactive Active
Size Small Very small
Chainable API Yes Yes
Time zone support No Via plugin
Locale support No Yes
Plugins No Many
Immutability No Yes
Community Small Large

Edits:

As I was still refactoring code, there were just a bunch of code with possible side effects, making it harder to actually know what was going on when reading.


I run the lint fix in the whole project, there were a bunch of reformat.


Much of the code were not encapsulated, and it was heavily tied to the previous libraries, it made way more difficult to migrate the library.


It appears that some components have a month property in them, yet it was tied as a XDate type, yet in one of the components, it was used as date, yet specified as just a month. So, there could have been hidden bugs in this part. Or just bad variable naming.


There seem to be a bunch of eslint and types errors, unrelated to anything I did, making it not possible to compile the project. That is not properly setup.


Btw many places lack testing, some tests were breaking and there was no clue where the issue was.

When adding tests, one should add unit tests covering all parts of it before adding any other testing that covers a different layer. This should be enforced in the project.

At this point tests in the project are all mixed up with no distinction of what type is what.

There should be a separation of files and folders naming per test type. Then enforce the coverage in the CI.


You should check LocaleConfig new implemntation. At the moment all the user need is setting the defaultLocale, the translation/i18n are automatically done by dayjs.

https://day.js.org/docs/en/i18n/changing-locale

https://day.js.org/docs/en/customization/customization

https://day.js.org/docs/en/i18n/i18n - https://github.com/iamkun/dayjs/tree/dev/src/locale


There are many bad implementations in the code, they dont make any sense and are far from clear. But, at least in the date part, it should be way clearer of what is happening. Still the implementations using it, might not be. Though I have simplified some.

It's like there were no reviews when they were added to the code.

Any project should have full enforcements of setup. Yet lint and format seems all out of place.

I see that this project lacks

If these two PRs are merged, I can work on another to deal with this, except on the coderabbit, of course.

@dougg0k dougg0k marked this pull request as draft July 4, 2025 01:43
@dougg0k dougg0k changed the title Replace xdate and momentjs feat: replace xdate and momentjs Jul 4, 2025
@dougg0k dougg0k marked this pull request as ready for review July 4, 2025 19:37
@dougg0k dougg0k marked this pull request as draft July 4, 2025 20:15
@dougg0k dougg0k marked this pull request as ready for review July 5, 2025 01:22
@x1mrdonut1x
Copy link

If you want to run a linter and/or code formatter on the entire repo when working on it, do this on a new branch and merge it separately. It's very hard and time consuming to actually review your feature/fix because there are formatting changes all over the place.

@dougg0k
Copy link
Author

dougg0k commented Jul 29, 2025

Well, if the project had a proper setup, it would work with the files needed only or not need at all. I ran the command which goes for the entire project. It should have been set in a different manner, if that wasnt the intent.

I had to run because I use biome as default in the IDE and that was affecting the code in different ways, that's why I said that lint and format was not properly setup in this project.

How running a command in the project going to be a hinder? It's clearly not been used or properly setup.

This PR might need an exception, as others would not have the problem anymore.

I even made a suggestion in my last part of the description. If this and the other PR are merged, I can work on the third and improve what is possible on the setup.

This PR took a good amount of work, it's not something that can just be easily re-done due to formatting or linting fixes. Ignore all that is not date related, as those will be related to format, lint or sorting.

This PR introduces a very good amount of improvement to the quality of the code. And by the lib immutability, less probable to introduce future bugs related to date.


I took a look, is not hard to review.

Most refactoring and used from impl will be from dateutils.ts, and the only changes will be on src/, still there are many that are just formats / lint / sorting.


You dont seem to be reviewer, why are you worried about it, anyhow? @x1mrdonut1x

@NorseGaud
Copy link

Screenshot 2025-08-27 at 9 34 29 PM Doesn't work

@dougg0k
Copy link
Author

dougg0k commented Aug 28, 2025

And how have you installed this PR changes? Seems to be in a different project. Plus unrelated to changes done.

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