WRO-582: [Sandstone-useScroll] Wrap the local functions in useScrollBase with useCallback #1247
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1247 +/- ##
===========================================
- Coverage 54.42% 54.00% -0.43%
===========================================
Files 136 136
Lines 6539 6653 +114
Branches 1915 1969 +54
===========================================
+ Hits 3559 3593 +34
- Misses 2328 2367 +39
- Partials 652 693 +41
Continue to review full report at Codecov.
|
| contextSharedState.set(`${id}.scrollPosition`, {x, y}); | ||
| } | ||
| } | ||
| }, [contextSharedState, props]); |
There was a problem hiding this comment.
props is always a new object, so props.id would be the right dep. It looks missing by accident. :)
There was a problem hiding this comment.
Yes, true. But if I add props.id I get this warning:
226:5 warning React Hook useCallback has a missing dependency: 'props'. Either include it or remove the dependency array react-hooks/exhaustive-deps
There was a problem hiding this comment.
Right. It could be okay in this line if we define id as destructed variable from props, but it looks unclear what impact this way may have. I'll look into this.
Enact-DCO-1.0-Signed-off-by: Stanca Pop [email protected]
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.Additional Considerations
Links
WRO-582
Comments