Memoize some functions in OptionsListUtils
#10239
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
melvin-bot
bot
requested review from
TomatoToaster
and removed request for
a team
|
|
luacmartins
requested changes
Aug 5, 2022
Is that bad? What does it mean? |
Not sure if this is related to this PR though? Does it also break on |
AFAIK npm changed its integrity checksum to sha512 so we should be using that going forward. Not sure if it's a bad thing or not, but we can be consistent and avoid changing this back and forth. I think updating npm and reinstalling the modules should fix this.
Ah this is just an observation that following the test steps on android result in that error. It also happens on main. |
|
Is the storage issue device level or an artificial limit we set in Onyx? |
|
I imagine this is some limit set in Onyx or SQLite, but I'm not 100% sure. I tried increasing both AsyncStorage_db_size_in_MB from 10MB to 1GB and I also see this in the console: |
|
Chatted with Marc and the integrity changes in |
luacmartins
previously approved these changes
Aug 5, 2022
|
Fixed them! |
luacmartins
previously approved these changes
Aug 5, 2022
TomatoToaster
previously approved these changes
Aug 8, 2022
|
I think we are good to merge once the conflicts are resolved |
johnmlee101
previously approved these changes
Aug 8, 2022
marcaaron
dismissed stale reviews from johnmlee101, TomatoToaster, and luacmartins
via
1ef1980
luacmartins
approved these changes
Aug 8, 2022
OSBotify
pushed a commit
that referenced
this pull request
Aug 8, 2022
Memoize some functions in `OptionsListUtils` (cherry picked from commit c15c2e7)
This was referenced Aug 8, 2022
roryabraham
reviewed
Aug 8, 2022
| @@ -70,6 +70,7 @@ | |||
| "htmlparser2": "^7.2.0", | |||
| "localforage": "^1.10.0", | |||
| "lodash": "4.17.21", | |||
| "memoize-one": "^6.0.0", | |||
There was a problem hiding this comment.
Out of curiosity, why did we choose to add a new lib instead of using _.memoize: https://underscorejs.org/#memoize ?
There was a problem hiding this comment.
_.memoize didn't work for me out of the box because it creates a hash from the first argument of the function (vs checking to see if all arguments are unique). memoize-one "just worked" so I rolled with it instead of trying to figure out the hash function stuff.
|
🚀 Cherry-picked to staging by @luacmartins in version: 1.1.88-6 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
1 similar comment
|
🚀 Cherry-picked to staging by @luacmartins in version: 1.1.88-6 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Details
Recalculating the Sidebar options when there are many reports is expensive and unnecessary. These long running functions contribute to some janky performance when reports are scaled up. Memoization can help reduce the number of recalculations that happen and unclog things a bit. But is not a final solution.
Fixed Issues (Related but does not completely solve)
#10248
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Screenshots
Web
Mobile Web
Desktop
iOS
Android