search

accordproject / web-components

React Components for Accord Project
Apache License 2.0
117 stars 94 forks source link

Use of JavaScript Date should probably be switched to Moment #10

Open jeromesimeon opened 4 years ago

jeromesimeon commented 4 years ago

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 commented 4 years ago

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

martinhalford commented 4 years ago

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 commented 4 years ago

@jeromesimeon I'm working on this.

jeromesimeon commented 4 years ago

@mttrbrts any thoughts on this issue?

mttrbrts commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

mttrbrts commented 4 years ago

Moment is already a dependency for concerto.

Concerto has moment-mini not moment as a dependency.

mttrbrts commented 4 years ago

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 commented 4 years ago

I have changed moment to moment-mini @mttrbrts

irmerk commented 3 years ago

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

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

jeromesimeon commented 3 years ago

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 commented 3 years ago

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

DianaLease commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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.