Skip to content

Persisting mutation batch state in LocalStorage #584

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.

Sign up for GitHub

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 6 commits into from
Mar 26, 2018
Merged

Persisting mutation batch state in LocalStorage #584

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
756eb47
Persisting mutation batch state in LocalStorage
schmidt-sebastian Mar 21, 2018
2d172ff
Addressing review comments
schmidt-sebastian Mar 23, 2018
1cb975b
Merge branch 'firestore-multi-tab' into multitab-mutationnotifications
schmidt-sebastian Mar 23, 2018
6826e49
Renaming delegate variables
schmidt-sebastian Mar 23, 2018
84420b6
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Mar 23, 2018
efd2c2f
Next round of review comments
schmidt-sebastian Mar 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions packages/firestore/src/local/shared_client_delegate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* Copyright 2018 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { BatchId } from '../core/types';
import { FirestoreError } from '../util/error';

/**
* Callback methods invoked by SharedClientState for notifications received
* from Local Storage.
*/
export interface SharedClientDelegate {
Copy link
Contributor

Choose a reason for hiding this comment

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

"Delegate" seems very iOS-y and I'd expect the delegate name to exactly match the class it's used with (i.e. SharedClientStateDelegate not SharedClientDelegate). I think the most JS-idiomatic way to do this would be for these to be events that you register for with sharedClientState.onNewPendingbatch(...) or similar. But I'm not opposed to wrapping them up together like this...

So naming options:

SharedClientStateChangeHandler
SharedClientStateCallbacks

You could also kinda' invert the way this works and have a set of change classes similar to WatchChange, and have consuming code like:

sharedClientState.onChange(change => { 
  if (change instanceof NewPendingBatchChange) { ... }
  else if (change instanceof ...) { ... }
})

That kind of appeals to me, but either way is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with naming this interface is that it will soon gain getActiveClients. It will then neither be a true delegate, and most certainly not a change handler or a list of callbacks. I went with SharedClientStateSyncer. I personally think 'Syncer' is a pretty awkward name, but at least it conveys that there is back and forth communication.

I also really like the way your .onChange API looks, but while I do agree that is appealing, it doesn't seem like the most straightforward way and will likely add some complexity.

/**
* Loads a pending batch from the persistence layer and updates all views.
*/
loadPendingBatch(batchId: BatchId): Promise<void>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these method names and descriptions are breaking the abstraction that this interface appears to be trying to create (to allow SharedClientState to generate notifications / callbacks without knowing how the consumer will make use of them). So the method should be handleNewPendingBatch() or similar, and leave the implementer to decide what to do with new batches.

Alternatively we could go the route of RemoteStore/SyncEngine/RemoteSyncer and not try to provide the illusion of an abstraction. I think then we'd call this SharedClientStateSyncer and have SharedClientState have a "syncEngine: SharedClientStateSyncer" property and rework the method names / comments with the expectation that SyncEngine will be the one and only implementer. I still think the method names need tweaking though. loadPendingBatch() should try to encompass the view updates, etc... Maybe just applyNewPendingBatch() or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason for this callbacks is to make testing feasible. My goal was not to create an abstraction layer on top of SyncEngine. It seems feasible to follow your suggestion here and mostly just look at this as an interface to SyncEngine. I renamed the internal members to reflect this.

I also went ahead and renamed this method to applyPendingBatch. I don't want to rely on the fact that this batch will always be unknown to SyncEngine.


/**
* Applies the result of a successful write of a mutation batch, updating all
* views and notifying all subscribers.
*/
applySuccessfulWrite(batchId: BatchId): Promise<void>;

/**
* Rejects a failed mutation batch, updating all views and notifying all
* subscribers.
*/
rejectFailedWrite(batchId: BatchId, err: FirestoreError): Promise<void>;
}
Loading