-
Notifications
You must be signed in to change notification settings - Fork 403
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
[GG-3938] Fixed date fields prefilling #536
Conversation
@@ -102,6 +117,12 @@ function getPrefilledTicketFields(fields: Fields): Fields { | |||
: ""; | |||
} | |||
break; | |||
case "due_at": | |||
case "date": | |||
if (isValidDate(sanitizedValue)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is not valid we just won't prefill the field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What else can we do, a part eventually showing a message in the console?
@@ -29,7 +29,26 @@ export function DatePicker({ | |||
value ? new Date(value as string) : undefined | |||
); | |||
|
|||
const formatDate = (value: Date | undefined) => { | |||
const dateTimeFormat = useMemo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it break the behaviour if this runs more than once?
Otherwise it feels like an over-optimization as we shouldn't need to memoize everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't but the Intl Formatters object creation is a slow process, as reported here: https://www.npmjs.com/package/intl-format-cache
It doesn't make much sense to recreate the instance at every re-render. Probably it would be better to use the linked package to create a memorized version for the entire app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This matters on the server side for big scale applications when actually paying for the compute.
For fun I created a quick JSPerf and this constructor averaged 0.032ms
. Even on a machine 100 times slower than mine it would still be single digit ms
. There's just no need for these type of optimizations when there's no perceivable impact.
Feel free to disregard my comment but IMO we're just polluting the code as my first thought when reading this code is "are we memoizing because this will break if it runs more than once?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All major i18n libraries are memoizing/caching Intl constructors (react-intl and i18next) and our internal library does the same.
I wanted to check as well the performance implication and I ran this benchmark on my phone and the average time of the fresh instance is ~0.1 ms. This means for example that if a page is displaying 20 formatted dates without any caching of the constructors, it will take 2ms just to instantiate these objects at every re-render. It doesn't seem like much time, but we should consider that for reaching 60 fps a frame needs to be rendered in ~16 ms, and there can be a lot of other things going on.
I would say that in general, it makes sense to cache Intl constructors. In was thinking that maybe it is not worth it in this case, since the formatting function is called only when the value changes and not at every re-render, but then I checked how it is used in Garden: https://github.com/zendeskgarden/react-components/blob/abb6e98cff5de709d67ef4769930d390d501d5b2/packages/datepickers/src/elements/Datepicker/Datepicker.tsx#L54. formatDate
and onChange
are used to create a memoized reducer and for this reason, we should wrap them in a useCallback
hook. I don't like adding useCallback
everywhere, but in this case, seems the most correct thing to do, so I changed the implementation of these functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All major i18n libraries are memoizing/caching Intl constructors (react-intl and i18next) and our internal library does the same.
They support server side rendering so it does matter as I mentioned.
I wanted to check as well the performance implication and I ran this benchmark on my phone and the average time of the fresh instance is ~0.1 ms.
I'm curious, how did you get to ~0.1ms
from that benchmark? And what phone did use?
I replicated it on this JS Benchmark which also prints the average runtime and I get it consistently around 0.035ms
on my 4+ yo phone. And they mention that the "actual ops/s might be higher in a real-world scenario".
This means for example that if a page is displaying 20 formatted dates without any caching of the constructors, it will take 2ms just to instantiate these objects at every re-render. It doesn't seem like much time, but we should consider that for reaching 60 fps a frame needs to be rendered in ~16 ms, and there can be a lot of other things going on.
I'm not arguing that it isn't much slower. I'm making a case that for our app, it just doesn't mater.
I also think that as developers we have the wrong priorities: our form takes seconds to load because of bloated js but here were are trying to shave off milliseconds for edge case forms with around 20 date fields.
I don't like adding useCallback everywhere, but in this case, seems the most correct thing to do, so I changed the implementation of these functions.
This component alone already has three :) I don't think we're wrong for trying to optimize - and one can argue that the useCallback
actually makes sense after your last finding - I just think we do it often at the expense of the code with no real benefit. Anyways, eventually react will solve this with the promised compiler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, how did you get to ~0.1ms from that benchmark? And what phone did use?
I was getting ~10.000 ops/s so 0.1 ms each. Today is 12.000 so a little more performant 😄 The phone is a Samsung Galaxy S22. Your benchmark is not 100% correct because the fresh instance part is creating a new instance and then reusing the shared one
Anyways, eventually react will solve this with the promised compiler
I hope so, it is a bit annoying to have to take these things into consideration. And Garden uses a lot of useCallback
s internally and this causes the user to wrap its own callbacks as well, and any dependant callback recursively
🎉 This PR is included in version 4.2.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
This PR fixes the prefilling of date fields in the Request Form.
Proper handling of time zones
The prefill value for date fields uses the
YYYY-MM-DD
format. We create a new Date object from that value in the Datepicker component, and this creates a date with the time at 00:00:00 UTC.For example, for a user in the
GMT-7
timezone,new Date('2025-10-25')
returnsFri Oct 25 2025 00:00:00 UTC
orFri Oct 24 2025 17:00:00 GMT-0700
in his timezone.Garden by default formats the date using the user timezone, so in this case we were showing
October 24, 2024
. This PR changes the formatting using UTC as a timezone.Added validation of date format
We were not validating the date passed in the URL. If the user passed a value like
2025-30-12
the form was breaking without any notice.Screenshots
Before:
After:
Checklist