Refactor ensureUpdateQueues to avoid returning a tuple#10437
Refactor ensureUpdateQueues to avoid returning a tuple#10437acdlite merged 4 commits intofacebook:masterfrom zombieJ:fixFiberUpdateQueueToDo
Conversation
acdlite
left a comment
There was a problem hiding this comment.
Hi! Thanks for the PR.
What I meant by that TODO was that I wanted to avoid allocating an array. Now you're allocating an object instead, which is no better.
The way I figured we could solve this is by storing the return values in a pair of module-level variables which the caller can read from.
|
Also let's please keep the naming of |
|
ok. Got it~ |
|
hi @acdlite, update the code. Please check. |
|
@zombieJ Hey sorry for the delay. So I'm realizing now that I should have been more specific. That's my bad. I would prefer not to have to do these checks twice: https://github.com/zombieJ/react/blob/4690e50c7d1d51761e5d128b9c446f0f2c1493e8/src/renderers/shared/fiber/ReactFiberUpdateQueue.js#L210-L213 I also don't want to allocate an object or array in order to return two queues. So what I think we should do is keep the body of let _queue1;
let _queue2;
function ensureUpdateQueues(fiber) {
// ...Same body as before...
// At the end, instead of returning an array, assign to _queue1 and queue2.
_queue1 = queue1;
// Return null if there is no alternate queue, or if its queue is the same.
_queue2 = queue2 !== queue1 ? queue2 : null;
}
function insertUpdate(fiber, update) {
// ...
ensureUpdateQueue(fiber);
const queue1 = _queue1;
const queue2 = _queue2;
}Hope this makes sense! |
|
@acdlite , |
|
@zombieJ Thanks for your work on this, and your patience in getting it landed! |
Refactor
ensureUpdateQueuesandinsertUpdate&addTopLevelUpdate.Use
queue&alternateQueueinstead ofqueue1&queue2.