Use of JavaScript Date should probably be switched to Moment about web-components

[jeromesimeon]

@DianaLease would you have cycles to pick this one up, now that we have updated Concerto?

Sure thing! To make sure I understand the issue - wherever we use the native Date object to create dates in concerto-ui, we instead want to create dayjs date objects since that's what we ended up using in Concerto, correct?

yep!

[jeromesimeon]

A way around testing for the time being is included in PR accordproject/concerto-ui#27

[martinhalford]

Yes - I came across the following Deprecation Warning while running the Cicero Server locally.

Deprecation warning: value provided is not in a recognized RFC2822 or ISO format. moment construction falls back to js Date(), which is not reliable across all browsers and versions. Non RFC2822/ISO date formats are discouraged and will be removed in an upcoming major release. Please refer to http://momentjs.com/guides/#/warnings/js-date/ for more info. Arguments: [0] _isAMomentObject: true, _isUTC: false, _useUTC: false, _l: undefined, _i: December 17, 2017 03:24:00, _f: undefined, _strict: undefined, _locale: [object Object] Error at Function.createFromInputFallback (/Users/martin/Development/Accord/cicero/packages/cicero-server/node_modules/moment-mini/moment.min.js:1:3368) at Yt (/Users/martin/Development/Accord/cicero/packages/cicero-server/node_modules/moment-mini/moment.min.js:1:21318) at Ot (/Users/martin/Development/Accord/cicero/packages/cicero-server/node_modules/moment-mini/moment.min.js:1:22029) at Tt (/Users/martin/Development/Accord/cicero/packages/cicero-server/node_modules/moment-mini/moment.min.js:1:22111) at new c.parseZone (/Users/martin/Development/Accord/cicero/packages/cicero-server/node_modules/moment-mini/moment.min.js:1:50429) at JSONPopulator.convertToObject (/Users/martin/Development/Accord/cicero/packages/cicero-server/node_modules/@accordproject/concerto-core/lib/serializer/jsonpopulator.js:221:26)

[raghav4]

@jeromesimeon I'm working on this.

[jeromesimeon]

@mttrbrts any thoughts on this issue?

[mttrbrts]

We should be careful to only add other dependencies where necessary as they bloat the bundle in downstream apps.

Providing a robust implementation only for this one function should not necessarily require bringing in a whole date library.

This library doesn't have proper locale support now, so we should provide values in an RFC or ISO compliant format

[jeromesimeon]

We should be careful to only add other dependencies where necessary as they bloat the bundle in downstream apps.

Providing a robust implementation only for this one function should not necessarily require bringing in a whole date library.

This library doesn't have proper locale support now, so we should provide values in an RFC or ISO compliant format

Moment is already a dependency for concerto.

[kanav-raina]

I would like to work on this, Kindly assign this to me.

[mttrbrts]

Moment is already a dependency for concerto.

Concerto has moment-mini not moment as a dependency.

[mttrbrts]

I'd also like to reconsider the DatePicker component that we use here, we shouldn't have to use non-standard date formats

[kanav-raina]

I have changed moment to moment-mini @mttrbrts

[irmerk]

Update on this: https://momentjs.com/docs/#/-project-status/

Maybe use https://moment.github.io/luxon/

[jeromesimeon]

Update on this: https://momentjs.com/docs/#/-project-status/

Maybe use https://moment.github.io/luxon/

Resolution on this, if any, should be consistent with whatever we decide on Concerto itself. See accordproject/concerto#205

[dselman]

@DianaLease would you have cycles to pick this one up, now that we have updated Concerto?

[DianaLease]

@DianaLease would you have cycles to pick this one up, now that we have updated Concerto?

Sure thing! To make sure I understand the issue - wherever we use the native Date object to create dates in concerto-ui, we instead want to create dayjs date objects since that's what we ended up using in Concerto, correct?

[DianaLease]

@dselman @jeromesimeon Looking into this more, I'm not sure it's actually necessary to introduce dayjs the way the current concerto-ui code is implemented (a bit has changed since this issue was first opened). As far as I can tell, we only use the native Date object in these two places:

Here we use it to get the ISO string, so we are just using the string, not the object.

web-components/packages/ui-concerto/src/utilities.js

Line 82 in 8e39155

return new Date().toISOString();

Here we use Date.now() which just returns a number.

web-components/packages/ui-concerto/src/formgenerator.js

Line 172 in 8e39155

timestamp: Date.now(),

Since we aren't actually using a date object created by the native Date() constructor, I don't think anything needs to be changed? Let me know if I am missing something!

[jeromesimeon]

@dselman @jeromesimeon Looking into this more, I'm not sure it's actually necessary to introduce dayjs the way the current concerto-ui code is implemented (a bit has changed since this issue was first opened). As far as I can tell, we only use the native Date object in these two places:

Here we use it to get the ISO string, so we are just using the string, not the object.

web-components/packages/ui-concerto/src/utilities.js

Line 82 in 8e39155

return new Date().toISOString();

Here we use Date.now() which just returns a number.

web-components/packages/ui-concerto/src/formgenerator.js

Line 172 in 8e39155

timestamp: Date.now(),

Since we aren't actually using a date object created by the native Date() constructor, I don't think anything needs to be changed? Let me know if I am missing something!

ok that's actually interesting. In my mind we should either consistently use an object or consistently use a string. Maybe what you're suggesting is that we should use Date.now().toISOString() in the second call listed?

(Apologies: I haven't really looked at this in details again, so I'm not even sure if I remember why I opened this issue)

[DianaLease]

@jeromesimeon The value of timestamp there is a number (typeof Date.now() === "number"). But if we wanted to make it a string to be consistent we could do new Date().toISOString() like we do in ui-concerto/src/utilities.js. When I do that while running storybook locally and add a DateTime field to the concerto form demo, everything seems to work fine. However, I can't really figure out how timestamp in ui-concerto/src/formgenerator.js is used, if it even is at all? Removing it entirely also seem to cause no issues 🤷‍♀️ I don't think it's actually related to the concerto DateTime type.