Skip to content
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

WRO-582: Wrap the local functions in useScroll with useCallback #3059

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

stanca-pop-lgp
Copy link
Contributor

Enact-DCO-1.0-Signed-off-by: Stanca Pop [email protected]

Checklist

  • I have read and understand the contribution guide
  • A CHANGELOG entry is included
  • At least one test case is included for this feature or bug fix
  • Documentation was added or is not needed
  • This is an API breaking change

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 useScroll with "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

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #3059 (a9cf407) into develop (0a973ef) will increase coverage by 0.09%.
The diff coverage is 53.54%.

❗ Current head a9cf407 differs from pull request most recent head 46878a5. Consider uploading reports for the commit 46878a5 to get more accurate results

@@             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              
Impacted Files Coverage Δ
packages/ui/useScroll/useScroll.js 41.51% <53.54%> (+1.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a973ef...46878a5. Read the comment docs.

Copy link
Contributor

@alexandrumorariu alexandrumorariu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

const
bounds = getScrollBounds(),
reachedEdgeInfo = {bottom: false, left: false, right: false, top: false};
// scroll start/stop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks that this function could be a function variable with useCallback. Do I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what is your concern. I'll look into it. (No good idea at a glance, though)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants