Allow user to unlisten to queries after shutdown. #2083
Conversation
| @@ -536,7 +536,6 @@ export class FirestoreClient { | |||
| } | |||
|
|
|||
| unlisten(listener: QueryListener): void { | |||
| this.verifyNotShutdown(); | |||
| this.asyncQueue.enqueueAndForget(() => { | |||
There was a problem hiding this comment.
This potentially calls into the SyncEngine. I wonder if we should guard this on the shutdown state and not call unlisten if we are shut down?
| @@ -536,7 +536,10 @@ export class FirestoreClient { | |||
| } | |||
|
|
|||
| unlisten(listener: QueryListener): void { | |||
| this.verifyNotShutdown(); | |||
| // Make unlisten a no-op if client is shut down. | |||
There was a problem hiding this comment.
Your code is so clear that I think we can remove this comment :)
There was a problem hiding this comment.
I actually want to keep it. The reason is FirestoreClient methods generally run verifyNotShutdown before doing anything. This one deviate from that.
I updated the comment to be more clear.
| @@ -536,7 +536,10 @@ export class FirestoreClient { | |||
| } | |||
|
|
|||
| unlisten(listener: QueryListener): void { | |||
| this.verifyNotShutdown(); | |||
| // Make unlisten a no-op if client is shut down. | |||
| if(this.clientShutdown) { | |||
There was a problem hiding this comment.
There is something going on with your formatter. This should be if (this.clientShutdown)
I would also slightly prefer if you wrote the inverse and removed the early return. Optional.
There was a problem hiding this comment.
Yeah, it's strange, the automated prettier commit did not make into the push somehow..
No description provided.