-
Notifications
You must be signed in to change notification settings - Fork 76
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
fix: slow render of location options #8562
Conversation
Oops! Looks like you forgot to update the changelog. When updating CHANGELOG.md, please consider the following:
|
Hi @rikukissa I think we will need this cherry-picked into a 1.6.3 hotfix branch so that we can give Uganda early access to it. They are going live on Feb 24th. Additionally any changes you are working on @tumbledwyer will likewise need cherry-picked. Maybe you discussed this already. |
packages/client/src/forms/utils.ts
Outdated
(sectionName, field, values) => { | ||
if ( | ||
field.type === SELECT_WITH_OPTIONS || | ||
field.type === DOCUMENT_UPLOADER_WITH_OPTION |
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 document uploader need this optimisation?
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.
The only one that really needs it is SELECT_WITH_DYNAMIC_OPTIONS
.
So you could potentially only memoize in that case.
Would we want to refactor just for that?
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 would reduce the regression risk tbh: the document uploader is (usually? always?) on a different page and it's options are probably fast to calculate, so doesn't have much of a performance improvement
@naftis please point this PR to develop with Milestone set to v1.6.3 |
Dynamic selects are the only ones causing performance issues so only memoizing them reduces the regression risk
chore: Refactor to only memoize for dynamic selects
Description
When we added the ability to create a 6th location level, we noticed that the address select performance in the Uganda configuration made using the form sluggish.
This issue was previously encountered in an old Cameroon project where selects had many thousands of values. Pyry fixed it partially in the Cameroon project but a minor tweak is required to complete the job. This PR solves the performance problem and should be cherry-picked into the 1.6.3 release branch. Uganda will go live with a githash (unofficial tagged hotifix) from that branch and if any issues are found in UAT, then we will upgrade from the same branch so as not to impact on core timelines for general UAT on the hotfix release.
Technical background
Basically the FormSectionComponent renders very slowly but it's not due to the sub-components rendering slowly. A JavaScript flamegraph looks like this, which tells us the option generating is slow. There are multiple different ways to solve this, best would probably be to not generate the options on every render, but this memoization is a hack hot fix that requires further looking into.