MPDX-6916: Replace Moment by reldredge71 · Pull Request #14 · CruGlobal/mpdx-react

reldredge71 · 2021-02-17T20:05:32Z

I decided to go with Luxon as a replacement to MomentJS: https://moment.github.io/luxon/index.html

vercel · 2021-02-17T20:05:35Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/cru/mpdx-react/DJaxDwFBkPCvXLty5BSJtJ2wm18q
✅ Preview: https://mpdx-react-git-mpdx-6916-replace-moment-cru.vercel.app

reldredge71 · 2021-02-17T20:10:19Z

+            Settings.resetCaches();
        });

        it('default', async () => {


This test is failing. For the WeeklyActivityTableCellDateRange, the expected value should be "Jan 26 - Feb 1", but it is now "Jan 27 - Feb 2". Unless Luxon defines a week as being Monday to Sunday, rather than Sunday to Saturday, I'm not sure why this is happening.

Actually I just found out, that's exactly what's happening: moment/luxon@6b145bf. Would it make more sense to update the test to the new "start of week" and "end of week" values, or make WeeklyActivity offset the values coming from Luxon?

Hmm ya the library just always returns Monday as the first day of the week. We might need to figure this out ourselves somehow. moment/luxon#373 (comment) has some code in a Gist that would work but that's annoying...

It seems weird displaying Monday as the first day of the week. But then again it probably seems weird for someone in another locale that Sunday is the first day of the week. But maybe we could consider these events as starting over every work week. Idk how we clearly show that to users as it's just a filter on the dashboard... Maybe a rolling 7 day window would make sense but idk if we can make that decision...

For explicitly setting Sunday to make US users happy, either of these could work:

.set({ weekday: 0 })

.startOf('week').minus({ days: 1 })

OzzieOrca

Code looks good :) Thanks for figuring this out!

I probably didn't make this clear enough in the ticket but Tatai was migrating to date-fns as he went along I think. I like Luxon but haven't really used date-fns. Sounds like you want to move forward with Luxon too. So we probably need to migrate from date-fns to Luxon too. Having 2 date libraries doesn't seem great. At this point it's probably better to do that second migration in another PR since this one is looking good and to keep thing cleaner.

OzzieOrca · 2021-02-19T00:31:59Z

+node_modules
+package-lock.json


How did you get files here? Yarn shouldn't make them... Kinda looks like you ran npm.

Yeah I ran yarn but I think there was something that wasn't installing properly until I ran npm. I can give it a try again to avoid npm altogether.

Oh I ran into this too initially. Did you set your yarn version locally to be Yarn 2? I followed the documentation to do that: https://yarnpkg.com/getting-started/install. The main command you're looking for is yarn set version berry but I had to upgrade to the newest version first in order to use yarn set.

Ya maybe you need a Yarn upgrade. I'm running 2.4. Yarn still uses the 1.x version even if you have 2.4 installed unless the project has v2 configured. https://github.com/CruGlobal/mpdx-react/blob/main/.yarnrc.yml#L5 should set that but maybe I have something configured wrong... As long as you have a current yarn version it should just work... Let me know if it doesn't.

OzzieOrca · 2021-02-19T00:34:33Z

    describe('MockDate', () => {
        beforeEach(() => {
-            MockDate.set(new Date(2020, 1, 1));
+            Settings.now = () => new Date(2020, 1, 1).valueOf();


Ooh this is nice to have included. Mocking dates has always been finicky.

OzzieOrca · 2021-02-19T00:35:51Z

        });
        afterEach(() => {
-            MockDate.reset();
+            Settings.resetCaches();


Should we do this in a global afterEach? Actually we could setup the mock date globally too...

Yeah we can probably do that. Where can I setup things to run globally on all jest tests?

OzzieOrca · 2021-02-19T00:38:25Z

+                                    const todayDate = DateTime.local().toISODate();
+                                    const date = DateTime.fromISO(contact.lateAt).toISODate();


Are these variables unused?

I guess not! I might have left that there after trying different solutions.

OzzieOrca · 2021-02-19T01:02:18Z

+            Settings.resetCaches();
        });

        it('default', async () => {


Hmm ya the library just always returns Monday as the first day of the week. We might need to figure this out ourselves somehow. moment/luxon#373 (comment) has some code in a Gist that would work but that's annoying...

It seems weird displaying Monday as the first day of the week. But then again it probably seems weird for someone in another locale that Sunday is the first day of the week. But maybe we could consider these events as starting over every work week. Idk how we clearly show that to users as it's just a filter on the dashboard... Maybe a rolling 7 day window would make sense but idk if we can make that decision...

For explicitly setting Sunday to make US users happy, either of these could work:

.set({ weekday: 0 })

.startOf('week').minus({ days: 1 })

OzzieOrca · 2021-02-19T01:07:29Z

+    const [startOfWeek, setStartOfWeek] = useState(DateTime.local().startOf('week'));
+    const [endOfWeek, setEndOfWeek] = useState(DateTime.local().endOf('week'));


An Interval transformed with mapEndpoints could work here. Seems a little cleaner since it's one state variable but maybe slightly more complicated.

reldredge71 · 2021-02-19T13:58:52Z

@OzzieOrca Yeah I think the ticket was clear enough, I think in the end Luxon looks nicer to me than date-fns. I also looked into DayJS since it seems like Moment with a smaller bundle size, but I think I like Luxon's API a little better. I can create another PR after this for ripping date-fns out as well.

OzzieOrca

Looks good :) Couple of trivial things.

OzzieOrca · 2021-02-19T20:36:34Z

+global.beforeEach(() => {
+  Settings.now = () => new Date(2020, 1, 1).valueOf();
+});
+
+global.afterEach(() => {


I think global is just the node global object variable (like window is in a browser). I don't think you need it as referencing a global variable directly should work. But if there are type errors or something, you can keep it.

Suggested change

global.beforeEach(() => {

Settings.now = () => new Date(2020, 1, 1).valueOf();

});

global.afterEach(() => {

beforeEach(() => {

Settings.now = () => new Date(2020, 1, 1).valueOf();

});

afterEach(() => {

OzzieOrca · 2021-02-19T20:37:51Z

@@ -1,5 +1,5 @@
 import React from 'react';
-import MockDate from 'mockdate';
+import { Settings } from 'luxon';


I think this is unused and is throwing a lint error.

OzzieOrca

📅 🕐 🎉

reldredge71 added 5 commits February 12, 2021 16:50

Remove Moment and install Luxon

9a10aaf

replace moment with luxon

d2ca726

add package-lock.json to .gitignore

ae41220

update tests

c90df4f

test fix

da168f7

reldredge71 requested review from OzzieOrca and TheNoodleMoose February 17, 2021 20:05

reldredge71 commented Feb 17, 2021

View reviewed changes

OzzieOrca suggested changes Feb 19, 2021

View reviewed changes

Merge main into origin/MPDX-6916-Replace-Moment and run Prettier on PR

4cfd228

vercel Bot deployed to Preview February 19, 2021 01:41 View deployment

reldredge71 added 3 commits February 19, 2021 10:34

revert .gitignore changes

a599bac

remove extra variables

1e5d3d2

use Interval

77a8fb2

vercel Bot deployed to Preview February 19, 2021 15:43 View deployment

fix WeeklyActivity tests

a66ca38

vercel Bot deployed to Preview February 19, 2021 16:46 View deployment

Make mock date global

1823f98

vercel Bot deployed to Preview February 19, 2021 18:41 View deployment

OzzieOrca suggested changes Feb 19, 2021

View reviewed changes

remove global label, remove Settings from file

fda7b69

vercel Bot deployed to Preview February 22, 2021 14:29 View deployment

OzzieOrca approved these changes Feb 22, 2021

View reviewed changes

reldredge71 merged commit 2ef278f into main Feb 22, 2021

reldredge71 deleted the MPDX-6916-Replace-Moment branch February 22, 2021 18:32

kegrimes mentioned this pull request May 27, 2026

MPDX-9626, MPDX-9627 MHA UAT fixes #1800

Merged

6 tasks

dr-bizz mentioned this pull request Jun 25, 2026

Agent-Review Core: config + index + learning layers #1858

Open

		const todayDate = DateTime.local().toISODate();
		const date = DateTime.fromISO(contact.lateAt).toISODate();

		const [startOfWeek, setStartOfWeek] = useState(DateTime.local().startOf('week'));
		const [endOfWeek, setEndOfWeek] = useState(DateTime.local().endOf('week'));

		node_modules
		package-lock.json

Uh oh!

Conversation

reldredge71 commented Feb 17, 2021

Uh oh!

vercel Bot commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OzzieOrca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reldredge71 commented Feb 19, 2021

Uh oh!

OzzieOrca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OzzieOrca left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vercel Bot commented Feb 17, 2021 •

edited

Loading