Skip to content
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

InvocationContext should be second parameter to HTTP trigger function #34

Closed
aaronpowell opened this issue Oct 30, 2022 · 5 comments · Fixed by #43
Closed

InvocationContext should be second parameter to HTTP trigger function #34

aaronpowell opened this issue Oct 30, 2022 · 5 comments · Fixed by #43
Assignees
Labels
breaking enhancement v4 🚀 We no longer use this label since v4 is now the default model.
Milestone

Comments

@aaronpowell
Copy link
Contributor

In the v4 programming model, we write HTTP trigger functions have a type signature of:

export type HttpHandler = (context: InvocationContext, request: HttpRequest) => FunctionResult<HttpResponse>;

But the InvocationContext is not something that you necessarily need, meaning that it's quite common to have it as an ignored argument, resulting in a TypeScript warning, ts(6133).

Propose solution

Reorder the type signature (and underlying code) so that the HttpRequest is the first argument, allowing the InvocationContext argument to dropped if it's not needed.

@ejizba ejizba added the v4 🚀 We no longer use this label since v4 is now the default model. label Oct 31, 2022
@ejizba
Copy link
Contributor

ejizba commented Oct 31, 2022

I think I agree with you - my gut is saying it's more likely people will ignore context than the input argument (aka request in the case of http). We just have to decide if it's worth breaking the existing Functions pattern - I will need to get some more opinions before implementing this change. Whatever we decide, though, my preference is to maintain consistency for all triggers.

@aaronpowell
Copy link
Contributor Author

Yes, it does beg the question of whether they should all move to the (trigger, context) parameter ordering for consistency.

Personal opinion - it makes more sense, as the thing that triggered the Function is probably the most important bit of information, and context is less relevant.

@ejizba
Copy link
Contributor

ejizba commented Nov 3, 2022

Talked with a few people during our team's sync meeting. They generally seemed to prefer context first, but everyone said they did not feel strongly either way.

A big reason they like context first is because every trigger gets it passed in and the type is always the same. By contrast, the trigger input type changes a bunch and it's not necessarily important for every trigger (they brought up durable which doesn't use an input at all and timer triggers which have a pretty useless input).

On the flip side, we were curious to compare to other languages and surprisingly (at least to me) we're the only one currently passing context first. Here is what I found as the arguments to an http trigger:

  • Python: req (context seems to be an optional second argument, but they don't include it in the default template)
  • .NET: request, logger (similar to python, there's an optional context argument this time as a third arg)
  • PowerShell: $Request, $TriggerMetadata
  • Java: request, context

@aaronpowell
Copy link
Contributor Author

Talked with a few people during our team's sync meeting. They generally seemed to prefer context first, but everyone said they did not feel strongly either way.

A big reason they like context first is because every trigger gets it passed in and the type is always the same. By contrast, the trigger input type changes a bunch and it's not necessarily important for every trigger (they brought up durable which doesn't use an input at all and timer triggers which have a pretty useless input).

But is the type really that important? JavaScript is inherently untyped, so it's not like there's much in the way of boxing/unboxing the type, it's always been order that's been important to JS Functions, and it's just the TypeScript definition that makes an assumption on what the type would be for the input.

I'd foresee you would have type definitions like (roughly, this was just scaffolded in markdown 😅):

export type FunctionHandler<TTrigger = unknown, TReturn = void> = (trigger: TTrigger, context: InvocationContext) => Promise<TReturn> | TReturn;

export type HttpFunctionHandler = FunctionHandler<HttpRequest, HttpResponse>;
export type TimerFunctionHandler = FunctionHandler<string, void>;

export namespace app {
    export function http(name: string, handler: HttpFunctionHandler): void;
    export function timer(name: string, handler: TimerFunctionHandler): void;
}

On the flip side, we were curious to compare to other languages and surprisingly (at least to me) we're the only one currently passing context first. Here is what I found as the arguments to an http trigger:

  • Python: req (context seems to be an optional second argument, but they don't include it in the default template)
  • .NET: request, logger (similar to python, there's an optional context argument this time as a third arg)
  • PowerShell: $Request, $TriggerMetadata
  • Java: request, context

I feel like that for dotnet (probably Java and PowerShell too) the order is less relevant as it's strongly typed and reflection can be used to determine the parameters and their order, but it is interesting to note that the are all trigger-first designed.

@ejizba
Copy link
Contributor

ejizba commented Nov 4, 2022

I don't disagree with you - was just passing along feedback.

For the information about other languages, I didn't realize that until after I had talked with my team. Once I told them, I think it swayed most folks in favor of this change. I'll let the decision bake for a little bit, but seems like we'll do it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking enhancement v4 🚀 We no longer use this label since v4 is now the default model.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants