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

Refactor ensureUpdateQueues to avoid returning a tuple #10437

Merged
merged 4 commits into from
Aug 28, 2017
Merged

Refactor ensureUpdateQueues to avoid returning a tuple #10437

merged 4 commits into from
Aug 28, 2017

Conversation

zombieJ
Copy link
Contributor

@zombieJ zombieJ commented Aug 11, 2017

Refactor ensureUpdateQueues and insertUpdate & addTopLevelUpdate.
Use queue & alternateQueue instead of queue1 & queue2.

Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

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

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.

@acdlite
Copy link
Collaborator

acdlite commented Aug 11, 2017

Also let's please keep the naming of queue1 and queue2

@zombieJ
Copy link
Contributor Author

zombieJ commented Aug 12, 2017

ok. Got it~

@zombieJ
Copy link
Contributor Author

zombieJ commented Aug 14, 2017

hi @acdlite, update the code. Please check.

@acdlite
Copy link
Collaborator

acdlite commented Aug 23, 2017

@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 ensureUpdateQueues mostly the same, but instead of returning a tuple at the end, mutate a module-level variable that both ensureUpdateQueues and insertUpdate have access to:

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!

@zombieJ
Copy link
Contributor Author

zombieJ commented Aug 28, 2017

@acdlite ,
Updated. Please check~

@acdlite acdlite merged commit c282a8e into facebook:master Aug 28, 2017
@acdlite
Copy link
Collaborator

acdlite commented Aug 28, 2017

@zombieJ Thanks for your work on this, and your patience in getting it landed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants