-
Notifications
You must be signed in to change notification settings - Fork 3k
feat: replace xdate and momentjs #2701
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
Open
dougg0k
wants to merge
44
commits into
wix:master
Choose a base branch
from
dougg0k:replace-xdate-moment
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains hidden or 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
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.
Edit:
All tests passing. Ready for review.
I suggest reading the comments, in this post.
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 and 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 2kb.
Here is a comparison between the previous and the new implementation.
XDate
Date
object but adds chainability and better parsing.Day.js
Summary Table
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.
You should check
LocaleConfig
new implemntation. At the moment all the user need is setting thedefaultLocale
, the translation/i18n are automatically done bydayjs
.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.