Adding goOnline/goOffline to the Web SDK #201
Conversation
schmidt-sebastian
requested review from
jshcrowthe,
mikelehen and
wilhuff
as code owners
schmidt-sebastian
force-pushed
the
mrschmidt-gooffline
branch
from
786f4b6 to
7f33883
Compare
schmidt-sebastian
force-pushed
the
mrschmidt-gooffline
branch
from
7f33883 to
9375faa
Compare
packages/firestore/package.json
Outdated
| "dev": "gulp dev", | ||
| "test": "run-p test:browser test:node", | ||
| "test:browser": "karma start --single-run", | ||
| "test:browser-debug" : "karma start --browsers=Chrome", |
There was a problem hiding this comment.
NIT: Can we call this test:browser:debug
NOTE: I'm going to be adding the same script to all of the SDKs, just wanted to be consistent with the naming
There was a problem hiding this comment.
This should also probably include the --auto-watch flag
i.e.
{
...
"test:browser:debug": "karma start --browsers Chrome --auto-watch"
...
}
| // Operations on the _firestoreClient don't block on _firestoreReady. Those | ||
| // are already set to synchronize on the async queue. | ||
| private _firestoreClient: FirestoreClient | undefined; | ||
| _firestoreClient: FirestoreClient | undefined; |
There was a problem hiding this comment.
Can you add a comment that this is public for testing? And explicitly use "public" to be consistent with the surrounding code?
| return this.syncEngine.handleUserChange(user); | ||
| } | ||
|
|
||
| /** Disabled the network connection. Pending operations will not complete. */ |
| /** Enables the network connection and requeues all pending operations. */ | ||
| public enableNetwork(): Promise<void> { | ||
| return this.asyncQueue.schedule(() => { | ||
| this.remoteStore.enableNetwork(); |
There was a problem hiding this comment.
I think we should return enableNetwork() instead of Promise.resolve() ?
There was a problem hiding this comment.
Yes, that make sense. I also made disableNetwork() return a Promise to align them better.
| .disableNetwork() | ||
| .then(() => { | ||
| return Promise.all([ | ||
| docRef.delete(), |
There was a problem hiding this comment.
Can you make the same change Gil did for iOS? (Sorry, I'm not awesome enough to do it for you)
There was a problem hiding this comment.
Ported the changes over.
| promises.push(docRef.set({ a: 1 })); | ||
| promises.push( | ||
| new Promise(resolve => { | ||
| done = resolve; |
There was a problem hiding this comment.
Could you use a Deferred (see other usages in this file) instead of this pattern?
There was a problem hiding this comment.
Yes, done (well, in the query test - see below)
| firestoreClient.enableNetwork().then(() => { | ||
| return docRef.get().then(snapshot => { | ||
| expect(snapshot.metadata.fromCache).to.be.false; | ||
| done(); |
There was a problem hiding this comment.
Actually, I'm fuzzy why you need the done thing at all. Why can't you just promises.push() this whole docRef.get().then(snapshot => ...) block? (you probably need to "return firestoreClient.enableNetwork()..." a couple lines up too)
There was a problem hiding this comment.
That is indeed a great simplification. And it turns out that I don't even need to keep all promises around, I just need a reference to the first promise.
|
|
||
| let done: () => void; | ||
| const promise = new Promise<void>(resolve => { | ||
| done = resolve; |
|
|
||
| coll.onSnapshot({ includeQueryMetadataChanges: true }, snapshot => { | ||
| if (!snapshot.empty && !snapshot.metadata.fromCache) { | ||
| done(); |
There was a problem hiding this comment.
Can you remove the listener?
There was a problem hiding this comment.
But but but... ok, done :)
4 participants
This is the JS port of firebase/firebase-ios-sdk#347