tweak(mobileForms): Improve performance #7263
Open
+3
−3
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
This is a very small change that should have a major impact on form performance. So the thing is, whenever
onLayout
is triggered, we update the state to keep track of where each field is, allowing us to scroll to said field if it had an error. I was trying to figure out what was causing ALL fields to re-render whenever I was expanding a dropdown and this was the culprit - opening a dropdown causes a layout update, which triggers the state setter.Usually this works fine-ish on medium spec devices, I think the delay is barely noticeable, though the select field is kinda wonky. Plus, we also do re-render every field after any field update (so with every keystroke!!). Unfortunately tackling that is way more complex, because some fields could theoretically depend on values from other fields - also, that's how it worked forever, so no reason to change THAT now for no apparent reason. And it's only a 'quick' refresh, say, a form with 10 fields will re-render 10 components so not that bad, right?
Anyway, opening a dropdown (or anything else that provokes a layout update) will cause all fields to re-render too, BUT, they will be re-rendered X times in a row, the X being the amount of layout shifts that occur, so the number of fields after that select field you just opened. This means, if we have a form with 10 fields and the first one is a select field, we will re-render the whole form 10 times, so 100 re-rendered components in a rapid fire succession.
With this work, this no longer happens, opening/closing a dropdown (or having layout shifts inside the form) no longer triggers a state update, saving us those hypothetical 100 re-renders. While state is preferred to manage state, a ref seems more appropriate here because we only want to keep track of the field position on the DOM, we gain absolutely nothing by having the components re-render as they will be displayed exactly the same.
Also, while this should be avoidable by explicitly managing if each children should re-render or not, that is quite troublesome because the high amount of props of all types, plus, we would be making a dull comparison as we know we don't want this to trigger a render, because we only want to be able to track the position silently.
Lastly, after this was done, I found out that we do the same thing on desktop, with the difference being that in desktop we save all actual DOM refs inside the "state" ref there and not the vertical position. I don't think we should unify the two, as in desktop we can probably get away with more memory usage.
TL;DR: lots of state updates for the same thing in a row causes a huge re-renders that don't need to happen, this should optimize forms performance.
NOTE: this applies to forms forms (so surveys).
Deploys