Skip to content
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

Replace moment-mini/moment date library with dayjs #4153

Merged
merged 2 commits into from
Feb 28, 2023

Conversation

aloisklink
Copy link
Member

📑 Summary

Replace Mermaid's dependency on moment/moment-mini with dayjs.

Moment is now in maintenance mode, and they don't recommend using it.
Dayjs is also much smaller, with the core library only being "2kB" when gzipped (although we have to use some plugins for backwards compatibility, which will increase the bundle size slightly).

Should hopefully resolve emersonbottero/vitepress-plugin-mermaid#33 (@emersonbottero, feel free to confirm).
Discussed in #4094 (comment)

📏 Design Decisions

Dayjs has almost exactly the same API as moment, and is still currently being maintained. Unlike moment, dayjs objects are immutable, which makes our life much easier (we never need to call dayjs.clone()), but we need to do a = a.add(1, "day") instead of just a.add(1, "day").

We can't use dayjs.duration, because unlike moment.duration, dayjs duration always degrade to ms. This causes issues with daylight savings, since it assumes that each day is 24 hours, when some days have 23/25 hours with daylight savings. (it also assumes that each month is 30 days).

However, dayjs.add(1, "d"); correctly adds 1 days, even when that day is only 23 hours long, so we can use that instead.

Issues:

All unit tests and e2e tests pass.

However, the following e2e test has a change:

it('should FAIL redering a gantt chart for issue #1060 with invalid date', () => {

Gantt-diagram-should-FAIL-redering-a-gantt-chart-for-issue-#1060-with-invalid-date diff

Does anybody have any ideas what might be different? I wonder if it's a daylight savings issue, since this Gantt chart seems to be one of the longest ones.

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added unit/e2e tests (if appropriate)
    • I've added a unit test that only runs in the 'America/Los_Angeles' timezone, that tests whether a task of duration 1d is 25 hours long on 2020-11-01, which because of daylight savings, has 25 hours in California.
  • 📓 have added documentation (if appropriate)
  • 🔖 targeted develop branch

Add a quick test that ensures `ganttDb` correctly adds `1d` (1 day),
even on days with 25 hours.

This test only runs if the test PC has the TZ='America/Los_Angeles'
set (California has daylight savings).

I've added a test to the GitHub Actions `test.yml` action too for this.
It should only add about 5 seconds to each test, so it isn't too bad.
Replace Mermaid's dependency on `moment` with `dayjs`.

[Moment is now in maintenance mode][1], and they don't recommend
using it.

[Dayjs][2] has almost exactly the same API as moment, and is still
curently being maintained. Unlike moment, dayjs objects are immutable,
which makes our life much easier, but we need to do
`a = a.add(1, "day")` instead of just `a.add(1, "day")`.

We can't use `dayjs.duration`, because unlike `moment.duration`,
[dayjs durations always degrade to ms][3].
This causes issues with daylight savings, since it assumes that each
day is 24 hours, when some days have 23/25 hours with daylight savings.
(it also assumes that each month is 30 days).

However, `dayjs.add(1, "d");` correctly adds 1 days, even when that
day is only 23 hours long, so we can use that instead.

[1]: https://momentjs.com/docs/#/-project-status/
[2]: https://day.js.org/
[3]: https://day.js.org/docs/en/durations/durations
@github-actions github-actions bot added the chore label Feb 26, 2023
@sidharthv96 sidharthv96 added this to the 10.0.1 milestone Feb 27, 2023
@sidharthv96 sidharthv96 merged commit c7bdc6a into mermaid-js:develop Feb 28, 2023
@emersonbottero
Copy link
Contributor

It didn't worked..

image

@aloisklink aloisklink mentioned this pull request Mar 8, 2023
@aloisklink
Copy link
Member Author

It didn't worked..

Hmmmmmm, we could try switching to the dayjs 2.0 ESM-only alpha, and that should fix it, but I'm a bit reluctant to do so, because it's only in alpha (it only has ~1000 downloads per week, compared to the 5.6 million downloads of the v1.11.7 release).

We could also bundle dayjs ourselves in mermaid. dayjs is tiny, so it shouldn't add much to our bundle sizes, but I'm always a bit reluctant to bundle anything, since it means Mermaid users can't independently update dayjs in case of any security issues/bugs.

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.

The requested module moment does not provide an export named default
3 participants