Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
melvin-bot
bot
requested review from
jasperhuangg
and removed request for
a team
|
I have read the CLA Document and I hereby sign the CLA |
mountiny
requested review from
danieldoglas,
marcaaron,
mountiny and
tgolen
marcaaron
left a comment
marcaaron
left a comment
There was a problem hiding this comment.
The changes LGTM overall. Great stuff @janicduplessis
Just one small request to aid in understanding the case where we do not batch
tgolen
left a comment
tgolen
left a comment
There was a problem hiding this comment.
Looks pretty straight forward
|
It seems like you're in pretty good hands here so I'm going to unassign myself since I don't have the best context on these changes. |
janicduplessis
force-pushed
the
@janic/init-batch
branch
from
36ffbbe to
7d684e6
Compare
|
@mountiny @marcaaron @tgolen Thanks for the review! I addressed your feedback. |
| return batchUpdatesPromise; | ||
| } | ||
|
|
||
| function batchUpdates(updates) { |
There was a problem hiding this comment.
| function batchUpdates(updates) { | |
| function queueUpdates(updates) { |
I think this is a clearer name for this function?
There was a problem hiding this comment.
but also it is not only queueing, it's queueing and maybe processing. Not sure. Maybe batchUpdates is explicative enough?
There was a problem hiding this comment.
This code was actually just moved and I didn't change it so maybe it would be better to keep it as is?
There was a problem hiding this comment.
Yeah, I didn't review the previous PR so I think it's NAB
|
@janicduplessis seems like tests are not happy |
janicduplessis
dismissed stale reviews from marcaaron and mountiny
via
847a314
|
@mountiny Strange, the tests are passing locally. I added a potential fix, can you run them again? |
|
Running |
|
@mountiny Looks like it passed 🎉 |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
I see the App is already on 1.0.87 so I think we can go ahead and merge this
6 participants
Details
Batch the initial hydration with the same mechanism introduced in #315. This is useful to make rendering of all connected components happen in the same batch. In this cases this reduces renders when opening a new chat in Expensify from ~100 to ~30.
Related Issues
N/A
Automated Tests
This doesn't really add new functionality, I just made sure the existing still pass.
Linked PRs
N/A