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

feat: Memoise token balance controler hook #6596

Merged
merged 34 commits into from
Jun 30, 2023

Conversation

NicolasMassart
Copy link
Contributor

@NicolasMassart NicolasMassart commented Jun 13, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

Create a hook that memoises the state request for token balances from TokenBalancesController.
This hook uses a deep object equality check to replace the default shallow comparison of useSelector.
Shallow comparison is not fitting the need for the complex data type we use here and it ends in some unwanted re-rendering or render misses due to data not being properly evaluated.

1. What is the reason for the change?
Need for performance improvement that prevents re-rendering when data hasn't deeply changed.

2. What is the improvement/solution?

  • Use deep data comparison with isEqual form Lodash lib on useSelector dependencies to trigger a new state retrieval.
  • Replace the useSelector call with this new hook in app/components/UI/Tokens/index.tsx
  • Implement the hook in order to start using the {data,loading,retry,error} return object pattern. Currently only {data} is implemented as the others require some deep controller changes out of the scope of this PR. This is a first step toward generalised controller hooks.
  • {data,loading,retry,error} return value type interface created with optional loading, retry and error as they are not yet implemented on controlers. But leads the way for future consistent use.
  • Implement tests that ensure the data is only returned when changed to prevent triggering of unnecessary render.

Issue

fixes https://github.com/MetaMask/mobile-planning/issues/808

QA test builds

iOS and Android builds available at https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/d0e62919-1ca8-4518-887c-0cf96b2afea3

Recording

Simulator.Screen.Recording.-.iPhone.12.Pro.-.2023-06-23.at.17.08.56.mov

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@NicolasMassart NicolasMassart marked this pull request as ready for review June 15, 2023 10:18
@NicolasMassart NicolasMassart requested a review from a team as a code owner June 15, 2023 10:18
@NicolasMassart NicolasMassart self-assigned this Jun 15, 2023
@NicolasMassart NicolasMassart added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jun 15, 2023
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

left some comments

Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM

@Cal-L Cal-L added needs-qa Any New Features that needs a full manual QA prior to being added to a release. team-mobile-client and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Jun 21, 2023
@cortisiko
Copy link
Member

hey @NicolasMassart ! Great work on getting this done. Can you add recordings of what you tested?

@NicolasMassart
Copy link
Contributor Author

@cortisiko I added recording and a link to Bitrise QA builds

@cortisiko cortisiko added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jun 29, 2023
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

This is QA passed! Feel free to merge whenever @NicolasMassart

@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Jun 29, 2023
@sonarcloud
Copy link

sonarcloud bot commented Jun 30, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@NicolasMassart NicolasMassart merged commit d475866 into main Jun 30, 2023
11 checks passed
@NicolasMassart NicolasMassart deleted the feat/808_token_balance_controler_hook branch June 30, 2023 18:46
@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-7.3.0 team-mobile-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants