Use of JavaScript Date should probably be switched to Moment

[jeromesimeon]

Both for stability and consistency with the rest of the Accord Project stack, it might be preferable to use moment for date creation rather than the custom use of the JavaScript Date() call.

This is notably affects code here: https://github.com/accordproject/concerto-ui/blob/f8b533d2e93d8526aa0bab708dcd0721f367c8ba/packages/concerto-ui-react/src/reactformvisitor.js#L243 or here: https://github.com/accordproject/concerto-ui/blob/f8b533d2e93d8526aa0bab708dcd0721f367c8ba/packages/concerto-ui-react/src/reactformvisitor.js#L504-L520

Also creation of dates in local timezone cannot be easily tested.

[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

[jolanglinais]

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 https://github.com/accordproject/concerto/issues/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?

[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!

[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. https://github.com/accordproject/web-components/blob/8e39155f3c39909867303ac1b399136aafe93028/packages/ui-concerto/src/utilities.js#L82

Here we use Date.now() which just returns a number. https://github.com/accordproject/web-components/blob/8e39155f3c39909867303ac1b399136aafe93028/packages/ui-concerto/src/formgenerator.js#L172

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.

https://github.com/accordproject/web-components/blob/8e39155f3c39909867303ac1b399136aafe93028/packages/ui-concerto/src/utilities.js#L82

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

https://github.com/accordproject/web-components/blob/8e39155f3c39909867303ac1b399136aafe93028/packages/ui-concerto/src/formgenerator.js#L172

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.