Limit use of unnecessary spread for performance #165
Conversation
melvin-bot
bot
requested review from
TomatoToaster
and removed request for
a team
|
|
||
| // eslint-disable-next-line no-param-reassign | ||
| prev[curr] = cachedValue; | ||
| return prev; |
There was a problem hiding this comment.
It could have a side effect theoretically. Because we are preserving the same ref to prev object, this operation will update the value in each reference. So I think the previous cache values will update in storageMap. I haven't tested it so it is just an assumption.
There was a problem hiding this comment.
Hmm maybe I'm missing something but the reference to prev seems to be the empty object declared as the reducer accumulator and shouldn't be updated anywhere else since it's declared in the reducer call, no?
|
Cool! I'll merge this to help solve the performance regression asap |
|
@marcaaron I'm already updating Onyx as part of https://github.com/Expensify/App/pull/10051/files, so I can just bump it up to this version |
|
Cool I've also added it in this Expensify/App#10239 which might be able to get deployed sooner |
Details
@luacmartins found some spread calls that were taking a long time in this thread.
Looking at the code and it seems like there is no clear reason to use the spread operator over simply building an object up one key at a time. When objects become large it is more and more expensive to do the spread as each iteration creates a copy.
Related Issues
https://github.com/Expensify/Expensify/issues/219425
Automated Tests
❌
Linked PRs
Let's add these changes into this PR