-
Notifications
You must be signed in to change notification settings - Fork 11
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
[T99687] BREAKING CHANGE(dayjs): Migrate moment to dayjs #150
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I tried it still successfully parsed the string (?)
using the same dayjs version, |
Sorry, forgot to mention that the issue occurs if we extend custom parse format (which we do) which is necessary to handle custom formats provided by the users.
will add these details on the changelog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm from me
This PR is pending due to known dayjs strict parsing bug iamkun/dayjs#929.
The PR is currently nor urgent as our front end projects don't have date time validator for now. We will revisit this PR once the need arises, and we'll see if there are fix or workaround by then. |
Compared to moment, dayjs is significantly smaller (73.1 kB vs 3 kB minified & gzipped) while still having similar date APIs. This should help reduce client bundle size for our front end projects which use cermati utils.
This change introduces a breaking behavior regarding invalid date input format.
Previously, parsing an incomplete input date using moment will fill the missing value. dayjs doesn't have this behavior. Considering adding
date-format
validation if your input is not guaranteed to have a valid format.