Skip to content

Change removeMutationBatch to remove a single batch #1148

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

Merged
merged 2 commits into from
Aug 20, 2018

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Aug 20, 2018

This is meant to make #1135 smaller and extracts the refactor that changes removeMutationBatches to removeMutationBatch, which only accepts a single batch at a time.

if (startIndex === 0) {
for (; queueIndex < queueCount; queueIndex++) {
if (batchIndex === 0) {
let queueIndex = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

endIndex? The queueIndex name was never any good and now it's even worse because we're not comparing the batch to remove against the queue contents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

mutation.key.path,
batch.batchId
);
this.removeCachedMutationKeys(batch.batchId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this called in the loop? Couldn't this be called outside the loop on mutations in the batch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this should just be called once. Fixed.

@schmidt-sebastian schmidt-sebastian merged commit 4625e84 into master Aug 20, 2018
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt-removemutationbatch branch August 20, 2018 21:42
@firebase firebase locked and limited conversation to collaborators Oct 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants