-
Notifications
You must be signed in to change notification settings - Fork 930
Add internal delete method to functions #1687
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I like the way this is implemented. Very clean. I just have a couple of questions about some details.
INTERNAL = { | ||
delete: (): Promise<void> => { | ||
this.deleteService(); | ||
return Promise.resolve(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that delete
is supposed to return a Promise
kind of implies to me that when the returned Promise
is resolved, then the service should be finished being deleted. But that won't be the case here. this.deleteService()
will have signaled the various function calls to cancel, but they won't be cancelled yet. Is that important?
It seems like it would be pretty annoying to have to implement some kind of countdown latch or something just to make sure all the functions have completed. So, what guarantees is delete
supposed to make?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at Firestore and Auth, it seems like delete
only guarantees that the deletion process has been started. Auth delete doesn't wait for anything before returning Promise.resolve()
, Firestore shuts down some services and waits for those to complete but doesn't cancel pending promises. I don't know if this is ideal, but it's consistent for now.
Made a small change to return the result of deleteService()
directly because it reads clearer, but it doesn't solve anything as far as timing.
The functions service lacks an
INTERNAL.delete()
method which I think is the cause of #1683. This adds a delete method which resolves a top-level promise on the service instance which should cancel any ongoing request promises.