-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Azure functions typescript #2487
Conversation
This will enable Apollo Server Typescript development for Azure Funcitons. https://azure.microsoft.com/en-us/blog/improving-the-typescript-support-in-azure-functions/
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.
Thank you for opening this PR, @michael-watson! The tests are currently failing because of omitted updates to the package-lock.json
for the added devDependencies
, however even once that's fixed, the compilation looks like it would still be failing because of some typing errors (when I checked this out locally):
packages/apollo-server-azure-functions/src/ApolloServer.ts:117:22 - error TS2345: Argument of type 'null' is not assignable to parameter of type 'string | Error | undefined'.
117 context.done(null, {
~~~~
packages/apollo-server-azure-functions/src/ApolloServer.ts:128:28 - error TS2339: Property 'originalUrl' does not exist on type 'HttpRequest'.
128 const path = req.originalUrl || '/';
~~~~~~~~~~~
packages/apollo-server-azure-functions/src/ApolloServer.ts:135:24 - error TS2345: Argument of type 'null' is not assignable to parameter of type 'string | Error | undefined'.
135 context.done(null, {
~~~~
packages/apollo-server-azure-functions/src/azureFunctionApollo.ts:60:22 - error TS2339: Property 'originalUrl' does not exist on type 'HttpRequest'.
60 url: request.originalUrl,
~~~~~~~~~~~
context: HttpContext, | ||
request: FunctionRequest, | ||
callback: (err?: any, output?: FunctionResponse) => void, | ||
context: Context, |
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.
Something isn't quite right with this new Context
type, or something is off with the way that we currently call context.done
for a normal (read: non-error) response.
I'm under the impression, based on these Azure docs a pattern would be to pass null
in the error parameter if there is no error. The previous version of this type (provided in the azureFunctions.ts
's Context
type which this PR removes) offered a done
which allowed that null
value, but is now producing errors as that's not a supported signature in @azure/functions
's typings.
My suspicion is that perhaps we need to change the way we invoke context.done
, but I'm not clear on why or when that changed (or if that's the desired move!).
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.
Yeah, this is a really good point. In testing, I've found that passing an empty string (""
) worked the same as null
in TypeScript; passing anything else breaks everything.
For this PR, I've changed null
to be ""
for now.
For the future, I've opened a PR against the @azure/functions
typings asking for guidance on this.
@abernix let me know if you think there is anything else we should do to cover the bases on this.
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.
@michael-watson Out of curiosity, what does passing undefined
do?
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.
It does work as well, but the Azure Functions team agreed with your assessment on the docs and I took all the credit.
I've updated our implementation to null
as I think semantically, undefined
doesn't make sense because it is defined. It's defined as no error, or null
. But I guess it really doesn't matter though, both are acceptable responses now.
- change `context.done` to provide empty string (`""`) instead of `null` - change `HttpRequest` code references to use `url` property which replaces the `originalUrl` property previously used - Install dev dependencies for azure typescript packages - Update @azure/functions to latest
We won't have to maintain an interface for this now.
@abernix let me know if there is anything else we need here |
Thanks again for this PR, @michael-watson! It's been published in |
TODO: