Simplify Listen API #3491
Simplify Listen API #3491
Conversation
💥 No ChangesetLatest commit: 5a0a67d Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂 If these changes should be published to npm, you need to add a changeset. This PR includes no changesetsWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Click here to learn what changesets are, and how to add one. Click here if you're a maintainer who wants to add a changeset to this PR |
schmidt-sebastian
force-pushed
the
mrschmidt/asyncobservercleanup
branch
2 times, most recently
from
ec8c6d8 to
ec5eddb
Compare
Binary Size ReportAffected SDKs
Test Logs
|
schmidt-sebastian
force-pushed
the
mrschmidt/asyncobservercleanup
branch
from
ec5eddb to
829de71
Compare
| }); | ||
| return () => { | ||
| observer.mute(); | ||
| this.asyncQueue.enqueueAndForget(() => this.eventMgr.unlisten(listener)); |
There was a problem hiding this comment.
@schmidt-sebastian Can you just confirm that it is indeed unnecessary to check for if (this.clientTerminated) before calling this.asyncQueue.enqueueAndForget()? I'm not familiar enough with AsyncQueue to know, but the old code avoids calling this.asyncQueue.enqueueAndForget() in the case that this.clientTerminated is false. The new code will call it regardless of the value of this.clientTerminated.
This comment also applies to addSnapshotsInSyncListener() below.
| this.eventMgr.removeSnapshotsInSyncListener(observer); | ||
| return Promise.resolve(); | ||
| }); | ||
| this.asyncQueue.enqueueAndForget(async () => |
There was a problem hiding this comment.
Why is it necessary to specify an async function here to this.asyncQueue.enqueueAndForget()? In the listen() method it just specifies a normal, non-async function. I see that this is consistent with the old code but the reason for the inconsistency within this file is non-obvious to me. Should all calls to enqueueAndForget() be async functions? It would appear so based on the signature of enqueueAndForget().
There was a problem hiding this comment.
There is a test already:
There was a problem hiding this comment.
Sorry - I replied to the wrong comment (but looks like you managed to restore the context). async makes the function return a Promise, even if the return of the value being returned is not a Promise. In the case above, "eventManager.listen" already returns a Promise.
This change is not needed, it just felt right to me. Manually returning Promise.resolve works as well.
There was a problem hiding this comment.
That makes sense. I forgot to check the return type of eventManager.listen(). With this explanation, I'd recommend to keep it as is, with the more modern async keyword.
schmidt-sebastian
force-pushed
the
mrschmidt/asyncobservercleanup
branch
from
4a93ee3 to
3e4c610
Compare
This is a cleanup PR before making FirestoreClient tree-shakeable.
Note that the "unsubscribe" callbacks now enqueue a callback on the async queue even after termination. This is a no-op and so we don't need to check for termination: https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/util/async_queue.ts#L312