WRO-582: Wrap the local functions in useScroll with useCallback#3059
WRO-582: Wrap the local functions in useScroll with useCallback#3059stanca-pop-lgp wants to merge 11 commits intodevelopfrom
Conversation
…hem to be defined before called
Codecov Report
@@ Coverage Diff @@
## develop #3059 +/- ##
===========================================
+ Coverage 48.15% 48.25% +0.09%
===========================================
Files 171 171
Lines 9697 9697
Branches 2602 2602
===========================================
+ Hits 4670 4679 +9
+ Misses 3945 3936 -9
Partials 1082 1082
Continue to review full report at Codecov.
|
packages/ui/useScroll/useScroll.js
Outdated
| const | ||
| bounds = getScrollBounds(), | ||
| reachedEdgeInfo = {bottom: false, left: false, right: false, top: false}; | ||
| // scroll start/stop |
There was a problem hiding this comment.
This comment is to distinguish code sections due to a large number of lines, and it looks that this line does not work its role now. If it is hard to gather functions for 'start' and 'stop', what about regrouping code and add some new comments for sections?
There was a problem hiding this comment.
Yes, I know it got messy and I tried not to affect the semantics logic. The thing is that wrapping functions in useCallback forced me to move them in order to be defined before being used. However, I will try to group them and add new comments.
| const status = mutableRef.current.overscrollStatus[orientation][edge]; | ||
| status.type = overscrollEffectType; | ||
| status.ratio = ratio; | ||
| function applyOverscrollEffect (orientation, edge, overscrollEffectType, ratio = overscrollDefaultRatio) { |
There was a problem hiding this comment.
It looks that this function could be a function variable with useCallback. Do I miss something?
There was a problem hiding this comment.
This PR is rather meant to be an attempt to solve the task, but I do not consider it complete. I got somewhat stuck and I would very much appreciate your input.
For this particular function, if I wrap it in useCallback I get this warning: "React Hook useCallback has a missing dependency: 'props'. Either include it or remove the dependency array. However, 'props' will change when any prop changes, so the preferred fix is to destructure the 'props' object outside of the useCallback call and refer to those specific props inside useCallback".
I chose not to do anything for the time being so that it does not affect the component's functionality. What would you suggest I do when getting this warning? (I get it for a lot of other functions if I wrap them in useCallback). Thank you!
There was a problem hiding this comment.
I see what is your concern. I'll look into it. (No good idea at a glance, though)
3 participants
Enact-DCO-1.0-Signed-off-by: Stanca Pop stanca.pop@lgepartner.com
Checklist
Issue Resolved / Feature Added
There are many local functions in custom Hooks of VirtualList and Scroller. When calling those custom Hooks, the functions are re-defined if those functions are not wrapped with useCallback. That would make JavaScript performance lower. By wrapping those functions with useCallback, we could prevent redefining them again if those functions's dependencies are not changed.
Resolution
Wrapped the local functions in
useScrollwith "useCallback" and reordered them to be called after being defined.The functions that haven't been wrapped in "useCallback" create a chain of dependencies that lead to the "start" and "stop" functions that eventually require to be wrapped in "useCallback". The problem is that these two call each other so there is no way of ordering them so that we don't get an error of not being defined before called.
Additional Considerations
Links
WRO-582
Comments