Conversation
jonahtanjz
left a comment
jonahtanjz
left a comment
There was a problem hiding this comment.
Thanks @tlylt and nice work on the tests 👍
LGTM! Fine with the migration :)
Just a quick follow-up question, are there any benefits for migrating moment in our use case?
I think of the main benefits is dayjs size is significantly smaller than moment but one of our nested dependencies still requires moment which means we can't really remove moment from the bundle size.
Thanks @jonahtanjz for the comment. I think the main motivations besides the general recommendation to move away from momentjs, are:
|
| </panel> | ||
|
|
||
| Full formatting reference available [here](https://momentjs.com/docs/#/displaying/format/). | ||
| Full formatting reference available [here](https://day.js.org/docs/en/parse/string-format#list-of-all-available-parsing-tokens) and [here](https://day.js.org/docs/en/plugin/advanced-format). |
There was a problem hiding this comment.
Does this have backward compatibility with moment? I haven't gone through the full list but it seems like Day.js misses quite a few things.
This might be a breaking change in that sense, my two cents would be to revert and revisit this until you guys would want to publish a v5.
Moment has committed to the following which should be sufficient ❔👀 for now:
However, since we understand that Moment is well established in millions of existing projects:
- We will address critical security concerns as they arise.
- We will release data updates for Moment-Timezone following IANA time zone database releases.
There was a problem hiding this comment.
Hi @ang-zeyu, wonder if you are asking backward compatibility with moment, specifically for the time formatting? I think this is the only place that we explicitly interface with moment, so it should probably be the only point that we need to focus on?
As to the formatting, I will do a check on the list. I think the typical ones are generally the same and perhaps if there's any issue it is with the more obscure formatting that we previously linked in the complete formatting reference, which is arguably less of an impact. It could indeed be a breaking change, if that is the case, I think we can definitely revert.
There was a problem hiding this comment.
Yes, the formatting strings. It seems even Luxon.js (supposedly a more complete 'inheritor' to moment) differs a fair bit.
There was a problem hiding this comment.
Here's the list for Moment vs Dayjs that I collated:
https://gdl5uplrx1.larksuite.com/sheets/shtuspALB1A7fRVMwJVHMWso6lP
Was initially worried about the support for "ddd", which is not explicitly mentioned in the dayjs docs but somehow works.
Other than that there are indeed some formats no longer supported.
This might be a breaking change in that sense, my two cents would be to revert and revisit this until you guys would want to publish a v5.
For this how do you recommend going about doing it since it has already been included in v4.0.1?
There was a problem hiding this comment.
ahh, I didn't realise its already released.
Normally, the best course of action would be to revert it quickly (@jonahtanjz) since we are following semantic versioning. (assuming consumers specify version as something like ^4.0, ~4.0 they would get the updates quickly)
Since this is a cli application that needs to be manually updated, another option is to document this blip in the changelog.
CI builds would still likely specify semantic versioning. I think best course of action is still to revert it.
There was a problem hiding this comment.
@ang-zeyu, confirming the rectification strategy:
- Revert the use of dayjs back to momentjs
- Undo the changes to the documentation
- Modify the test cases to retain relevant ones
After the new PR is raised and merged, do we release a 4.0.2 or are we making some kind of direct amendment on 4.0.1?
There was a problem hiding this comment.
After the new PR is raised and merged, do we release a 4.0.2 or are we making some kind of direct amendment on 4.0.1?
Most likely we will have to release 4.0.2 to revert this change? If I am not wrong, I don't think it's possible to make amendments to a published package.
From npm docs:
The publish will fail if the package name and version combination already exists in the specified registry.
Once a package is published with a given name and version, that specific name and version combination can never be used again, even if it is removed with npm unpublish.
There was a problem hiding this comment.
Sure thanks @jonahtanjz! I will make a revert PR soon... guess we can use the next release to deal with this and the modal scroll issue
4 participants
What is the purpose of this pull request?
Overview of changes:
https://momentjs.com/docs/#/-project-status/recommendations/
Anything you'd like to highlight/discuss:
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
Migrate moment to dayjs
Checklist: ☑️