-
Notifications
You must be signed in to change notification settings - Fork 917
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
[cleanup] remove unexpected await
s
#2806
Conversation
@@ -424,7 +424,7 @@ export class FunctionsEmulator implements EmulatorInstance { | |||
added = await this.addPubsubTrigger(this.args.projectId, definition); | |||
break; | |||
case Constants.SERVICE_AUTH: | |||
added = await this.addAuthTrigger(this.args.projectId, definition); | |||
added = this.addAuthTrigger(this.args.projectId, definition); |
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.
This one is a bit suspicious to me. I guess because auth is in-process it can be added synchronously?
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.
yup - it's a sync function, not async, so no need to await
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.
Two small comments
@@ -25,7 +25,7 @@ export interface FunctionProxyRewrite { | |||
export default function( | |||
options: FunctionsProxyOptions | |||
): (r: FunctionProxyRewrite) => Promise<RequestHandler> { | |||
return async (rewrite: FunctionProxyRewrite) => { | |||
return (rewrite: FunctionProxyRewrite) => { |
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.
nit: You may want to wrap the whole function body around new Promise
to keep the behavior the same. Before, any errors in the function body will be returned as a rejected Promise. Now it just synchronously throws (which can be really bad), unless you wrap everything.
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.
Makes sense. I'll wrap it. Thanks.
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.
LGTM (with nit above)
Description
Removing the
await-thenable
warning and fixing all the errors.Scenarios Tested