Port waitForPendingWrites from android #2081

wu-hui · 2019-08-13T19:49:42Z

Port waitForPendingWrites

schmidt-sebastian

This looks like it does the trick, but I left some improvement suggestions. Thanks!

schmidt-sebastian · 2019-08-14T04:20:38Z

packages/firestore/src/api/database.ts

+   * Waits until all currently pending writes for the active user have been acknowledged by the
+   * backend.
+   *
+   * The returned Promise completes immediately if there are no outstanding writes. Otherwise, the


s/completes/resolves/

schmidt-sebastian · 2019-08-14T04:22:45Z

packages/firestore/src/api/database.ts

+   * acknowledged by the backend.
+   */
+  _waitForPendingWrites(): Promise<void> {
+    return this._firestoreClient!.waitForPendingWrites();


Please call ensureClientConfigured() to make sure this works even when it is the first operation that is scheduled.

schmidt-sebastian · 2019-08-14T04:24:30Z

packages/firestore/src/core/firestore_client.ts

@@ -522,6 +522,20 @@ export class FirestoreClient {
    });
  }

+  /**
+   * Returns a `Promise` resolves when all the pending writes at the time when this method is called


I would remove the backticks for Promise. It's just a normal term in JS, not a class or method name (also it should be "Promise that resolves");

Also: s/when all the pending writes at the time/when all writes that were pending at the time/

schmidt-sebastian · 2019-08-14T04:25:39Z

packages/firestore/src/core/sync_engine.ts

@@ -602,6 +609,62 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
    }
  }

+  /**
+   * Takes a snapshot of current mutation queue, and register a user callback


s/register/registers

schmidt-sebastian · 2019-08-14T04:26:03Z

packages/firestore/src/core/sync_engine.ts

@@ -602,6 +609,62 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
    }
  }

+  /**
+   * Takes a snapshot of current mutation queue, and register a user callback
+   * which will be called when all those mutations in the snapshot are either


What do you mean by "snapshot" here? Snapshot usually means something different in our code base.

schmidt-sebastian · 2019-08-14T04:42:18Z

packages/firestore/test/integration/api/database.test.ts

+
+      // pending writes can receive acknowledgements now.
+      await firestore.enableNetwork();
+      await expect(pendingWrites).to.be.eventually.fulfilled;


await expect(pendingWrites).to.be.eventually.fulfilled is the same as await pendingWrites.

schmidt-sebastian · 2019-08-14T04:42:56Z

packages/firestore/test/integration/api/database.test.ts

+
+      mockCredentialsProvider.changeUserTo(new User('user_1'));
+
+      await expect(awaitPendingWrite).to.be.eventually.rejected;


Please do test the error message as well. It will also make the test much clearer.

schmidt-sebastian · 2019-08-14T04:44:01Z

packages/firestore/test/integration/api/database.test.ts

+        await firestore.disableNetwork();
+
+        const awaitPendingWrites = waitForPendingWrites(firestore);
+        await expect(awaitPendingWrites).to.be.eventually.fulfilled;


This is the same test case as above. In general, it is usually better to have smaller and more focused test cases. Maybe we should remove the "offline but no writes" case from above and only test it here?

schmidt-sebastian · 2019-08-14T04:45:08Z

packages/firestore/test/integration/util/helpers.ts

+
+  private listener: CredentialChangeListener | null = null;
+
+  changeUserTo(user: User): void {


Do you think it makes sense to call this handleUserChange to match SyncEngine?

handleUserChange implies it's handling something, which is not true. Changed to triggerUserChange instead.

schmidt-sebastian · 2019-08-14T04:46:21Z

packages/firestore/test/integration/util/helpers.ts

+): Promise<void> {
+  const mockCredentialsProvider = new MockCredentialsProvider();
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  const settings: any = { ...DEFAULT_SETTINGS };


You can potentially remove the any:

const settings = {... DEFAULT_SETTINGS, credentials: { client: mockCredentialsProvider, type: 'provider' }}

schmidt-sebastian

LGTM with some comment nits. I do want you to make registerPendingWritesCallback non-async though :)

schmidt-sebastian · 2019-08-14T18:13:15Z

packages/firestore/src/core/sync_engine.ts

+      return Promise.resolve();
+    }
+
+    const callbacks = this.pendingWritesCallbacks.get(largestBatchId) || [];


You can invoke yarn build in "packages/firestore". It will spit out a couple different files into "packages/firestore/dist"

In packages/firestore/dist/index.esm.js, your current code looks as such:

callbacks = this.pendingWritesCallbacks.get(highestBatchId) || []; callbacks.push(callback); this.pendingWritesCallbacks.set(highestBatchId, callbacks);

It looks like we have to specify different compiler settings to make my suggestion work (--downlevelIterators)/ Since it is just an optional suggestion, let's keep it as is.

schmidt-sebastian · 2019-08-14T18:13:48Z

packages/firestore/src/api/database.ts

+   * backend.
+   *
+   * The returned Promise resolves immediately if there are no outstanding writes. Otherwise, the
+   * Task waits for all previously issued writes (including those written in a previous app


s/Task/Promise/

schmidt-sebastian · 2019-08-14T18:14:08Z

packages/firestore/src/api/database.ts

+   *
+   * Any outstanding `waitForPendingWrites()` Promises are rejected during user changes.
+   *
+   * @return A `Promise` which resolves when all currently pending writes have been


Nit: remove backticks.

schmidt-sebastian · 2019-08-14T18:14:56Z

packages/firestore/src/core/firestore_client.ts

@@ -522,6 +522,21 @@ export class FirestoreClient {
    });
  }

+  /**
+   * Returns a Promise that resolves when when all writes that were pending at the time when this


s/when when/when/
s/at the time when/at the time/

schmidt-sebastian · 2019-08-14T18:15:03Z

packages/firestore/src/core/firestore_client.ts

@@ -522,6 +522,21 @@ export class FirestoreClient {
    });
  }

+  /**
+   * Returns a Promise that resolves when when all writes that were pending at the time when this
+   * method is called received server acknowledgement. An acknowledgement can be either acceptance


schmidt-sebastian · 2019-08-14T18:16:27Z

packages/firestore/src/core/sync_engine.ts

@@ -450,6 +452,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
      (this.isPrimary && source === OnlineStateSource.RemoteStore) ||
      (!this.isPrimary && source === OnlineStateSource.SharedClientState)
    ) {
+      this.assertSubscribed('applyOnlineStateChange()');


Is this intended?

Yes. This was missing but should be present. Otherwise this.syncEngineListener!.* might hit errors.

schmidt-sebastian · 2019-08-14T18:17:43Z

packages/firestore/src/core/sync_engine.ts

@@ -602,6 +609,59 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
    }
  }

+  /**
+   * Registers a user callback with all the pending mutations at the moment of calling.


Can you update this to:

Registers a user callback that resolves when all pending mutations at the moment of calling are acknowledged .

You can then remove the second sentence.

schmidt-sebastian · 2019-08-14T18:18:24Z

packages/firestore/src/core/sync_engine.ts

+   * if there are any.
+   */
+  private triggerPendingWritesCallbacks(batchId: BatchId): void {
+    (this.pendingWritesCallbacks.get(batchId) || []).forEach(callback => {


I learned something today :)

schmidt-sebastian · 2019-08-14T18:20:07Z

packages/firestore/test/integration/api/database.test.ts

+        // Prevent pending writes receiving acknowledgement.
+        await firestore.disableNetwork();
+
+      // `awaitsPendingWrites` is created when there is no pending writes, it will resolve


This looks like it needs reformatting.

schmidt-sebastian · 2019-08-14T18:20:44Z

packages/firestore/test/integration/api/database.test.ts

+
+      // `awaitsPendingWrites` is created when there is no pending writes, it will resolve
+      // immediately even if we are offline.
+        const awaitPendingWrites = waitForPendingWrites(firestore);


Suggest to remove the redundant variable here.

schmidt-sebastian · 2019-08-14T18:30:31Z

I do want you to make registerPendingWritesCallback non-async though :)

Looks like this is not possible as getHighestUnacknowledgedBatchId returns a Promise. Thanks for pointing this out.

waitForPendingWrites + allow-unlisten-after-shutdown.

5897991

wu-hui requested a review from schmidt-sebastian August 13, 2019 20:12

wu-hui assigned schmidt-sebastian Aug 13, 2019

wu-hui added the api: firestore label Aug 14, 2019

schmidt-sebastian reviewed Aug 14, 2019

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Aug 14, 2019

addressing comments

2715154

wu-hui changed the title ~~waitForPendingWrites + allow-unlisten-after-shutdown.~~ Port waitForPendingWrites from android Aug 14, 2019

wu-hui assigned schmidt-sebastian and unassigned wu-hui Aug 14, 2019

schmidt-sebastian approved these changes Aug 14, 2019

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Aug 14, 2019

addressing comments #2

7d199b0

wu-hui merged commit f18b0fb into master Aug 15, 2019

firebase locked and limited conversation to collaborators Oct 8, 2019


		mockCredentialsProvider.changeUserTo(new User('user_1'));

		await expect(awaitPendingWrite).to.be.eventually.rejected;


		private listener: CredentialChangeListener \| null = null;

		changeUserTo(user: User): void {

Port waitForPendingWrites from android #2081

Port waitForPendingWrites from android #2081

Conversation

wu-hui commented Aug 13, 2019 • edited Loading

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian commented Aug 14, 2019

wu-hui commented Aug 13, 2019 •

edited

Loading