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

Return type for functionRunner #43

Closed
BenJohnCarson opened this issue Jun 15, 2023 · 13 comments
Closed

Return type for functionRunner #43

BenJohnCarson opened this issue Jun 15, 2023 · 13 comments

Comments

@BenJohnCarson
Copy link

Currently the functionRunner has a return type of Promise<any>.
This results in a few typescript-eslint errors:

Assigning result :
Screenshot 2023-06-15 at 10 57 23

Trying to type with Promise<Context> :
Screenshot 2023-06-15 at 11 01 02

For now we've got around this by adding an overload to our project:

import * as stubAzureFunctionContext from 'stub-azure-function-context';

declare module 'stub-azure-function-context' {
  export function functionRunner(
    azFunction: AzureFunction,
    bindingDefinitions?: stubAzureFunctionContext.BindingDefinition[] | string,
    bindingData?: Record<string, stubAzureFunctionContext.Binding>,
    augmentContext?: stubAzureFunctionContext.AugmentContextCallback,
  ): Promise<Context>;
}

It'd be ideal if the type were narrowed in the package itself. Is there a reason it's currently any? Is Context a sufficient type?

@dhensby
Copy link
Collaborator

dhensby commented Jun 15, 2023

The return type of the function runner is not a Context so we shouldn't be setting it so at all.

It is any (though I suppose could be unknown) because the function can return... anything.

If you see the docs, it can be an HTTP response, or a void function, etc. I'm not aware of any time the function would (or should) return the context.

I'm not really clear on what the linting error is there, perhaps because the screen shots don't show enough context, but I can't tell why or where you'd be wanting to assert the type is a Context. Do you have some more complete code to show us?

@dhensby
Copy link
Collaborator

dhensby commented Jun 15, 2023

OK - right, my confusion here is that the functionRunner returns the context if the function itself doesn't actually return anything.

So if your underlying function is not returning a value, you'll get the context back, but if it does return something then you get that back. So we can't just declare the return type as a Context because the return type is a Context in some scenarios, but not in any where the function has a return value.

@BenJohnCarson
Copy link
Author

Ah ok interesting. In that case would allowing passing a generic type for the return of the underlying function be a possible option? Perhaps having it default to unkown if not supplied. Something like :

export async function functionRunner<T = unkown>(
    azFunction: AzureFunction, 
    bindingDefinitions: BindingDefinition[] | string = [], 
    bindingData: Record<string, Binding> = {}, 
    augmentContext?: AugmentContextCallback
): Promise<T> { ...

@dhensby
Copy link
Collaborator

dhensby commented Jun 16, 2023

It could be generic, or maybe even using the return type of AzureFunction itself?

@dhensby
Copy link
Collaborator

dhensby commented Jun 16, 2023

OK - there doesn't seem to be a way to infer the return type, which would be a bit nicer than forcing the use of a generic.

I've added it as a generic in #44, but I would prefer to add a conditional return type if we could evaluate if the azure function was a void function or not...

@BenJohnCarson
Copy link
Author

Yeh that'd be ideal. I'm not sure it's possible though because of the AzureFunction type that comes from the @azure/functions package (here). There it's always typed as Promise<any> | void so unless the consumer passed a function to functionRunner that used a custom type other than AzureFunction it'd always evaluate to the same type.

@dhensby
Copy link
Collaborator

dhensby commented Jun 19, 2023

Well, if a function is declared to return Promise<string> that is a valid AzureFunction which has been narrowed, so it should be possible in theory to obtain that narrowed return type... though I've not been able to 😅

@dhensby
Copy link
Collaborator

dhensby commented Jun 19, 2023

OK - I think I've managed to get it working so it will infer the return type successfully.

Do you think you could install the branch that #44 is based on and see if that resolves your problems?

@BenJohnCarson
Copy link
Author

Unfortunately it still defaults to a return type of Promise<any>.

Screenshot 2023-06-19 at 15 38 11

It does work if I tweak it to this (adding the any | void):

export declare function functionRunner<T extends AzureFunction = AzureFunction>(
  azFunction: T,
  bindingDefinitions?: BindingDefinition[] | string,
  bindingData?: Record<string, Binding>,
  augmentContext?: AugmentContextCallback,
): Promise<Awaited<ReturnType<T>> extends any | void ? Context : ReturnType<T>>;

But then I don't think it's possible to get it to have a return type of anything other than Context as extends any | void I think always evaluates to true. I do think the AzureFunction type is the problem here as it doesn't allow determining the return type

@BenJohnCarson
Copy link
Author

It may just be that the way our team has written our functions is a little niche and perhaps not quite the intended approach. e.g. our functions will always have a return type of Promise<void> because we just set the response to context.res:

export const httpTrigger: AzureFunction = async (context): Promise<void> => {
  context.res = {
    status: 200,
    body: {
      status: 'Healthy',
    },
  };
};

@dhensby
Copy link
Collaborator

dhensby commented Jun 19, 2023

When I check it locally the typings seem to be resolving even for your example signature, I've even turned on the @typescript-eslint/no-unsafe-assignment rule and it passes for your example.

image

So it doesn't like the default any type that is inferred from the stub, but it's happy with the more specific typings if the function is a void function or returns a narrower type.

@BenJohnCarson
Copy link
Author

Yeh looks like it works perfectly if I change my function to not use the AzureFunction type and instead explicitly type it. For some reason it ignores/overrides the declared return type when typing the function with AzureFunction:

// triggers eslint rule in test, functionRunner return type any
export const httpTrigger: AzureFunction = async (context): Promise<void> => {}

// doesn't trigger eslint rule, functionRunner return type Context
export const httpTrigger = async (context: Context): Promise<void> => {}

I'm happy with this solution if you are!

@dhensby
Copy link
Collaborator

dhensby commented Jun 20, 2023

fixed in #44 and released in v2.2.1

@dhensby dhensby closed this as completed Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants