Split programming model out of worker (library side) #1
Conversation
There was a problem hiding this comment.
In general, lgtm :) I left a few nits/questions, but nothing I wanna block over. Also assuming that most of the code is copied over from the old worker, while modifying types to use types provided by the core API, so didn't spend too much analyzing the details of what the invocation model does
| import { AzureFunction, Context } from '@azure/functions'; | ||
| import * as coreTypes from '@azure/functions-core'; | ||
| import { | ||
| CoreInvocationContext, | ||
| InvocationArguments, | ||
| RpcInvocationResponse, | ||
| RpcLog, | ||
| RpcParameterBinding, | ||
| } from '@azure/functions-core'; |
There was a problem hiding this comment.
These imports are weird to me.
When importing AzureFunction and Context from @azure/functions, are we importing them from the current npm package (i.e. the worker), or are we importing them from ourselves?
Also, why are we importing * as coreTypes from @azure/functions-core, while excluding CoreInvocationContext, InvocationArguments, etc., and importing them on their own?
There was a problem hiding this comment.
When importing
AzureFunctionandContextfrom@azure/functions, are we importing them from the current npm package (i.e. the worker), or are we importing them from ourselves?
I'm a bit confused by this question. The "current npm package" is not the worker, it's the library. And it really just translates to the types/index.d.ts file in this repo (relevant line in tsconfig here). We import two-ish things from "ourselves" - for things that go away after you compile to JavaScript (think interface and type) we will import from the types file in this repo (aka @azure/functions). For other things that are real source code and actually stay when compiled to javascript (class, const, function etc.) we will import from the src folder directly
Also, why are we importing
* as coreTypesfrom@azure/functions-core, while excludingCoreInvocationContext,InvocationArguments, etc., and importing them on their own?
This is easier to answer. I prefer "importing them on their own". However, I use * as coreTypes for lines like this where the class name and interface name are the same and would conflict with each other:
export class InvocationModel implements coreTypes.InvocationModel {
| const info = this.#funcInfo; | ||
|
|
||
| // Allow HTTP response from context.res if HTTP response is not defined from the context.bindings object | ||
| if (info.httpOutputName && context.res && context.bindings[info.httpOutputName] === undefined) { |
There was a problem hiding this comment.
I'm assuming this is just copying old code, but just a javascript question here: when you type this condition like this, does this check that each of info.httpOutputName, context,res, etc., are each === undefined, or does this first evaluate info.httpOutputName as a truth value (i.e., info.httpOutputName !== undefined), then does the same for context.res, etc.?
There was a problem hiding this comment.
Yeah just copying code.
First of all, info.httpOutputName doesn't just check === undefined. The condition will be false for any false-y value like undefined, null, '', 0, false and maybe more. Sometimes this can lead to unexpected behavior and weird bugs, but we have lint rules in place to help with that.
Secondly, if the first condition returns false, it will not check the following conditions for an && condition. So if info.httpOutputName is false, it will not check context.res.
I think that's what you were asking...
| import * as coreTypes from '@azure/functions-core'; | ||
| import { CoreInvocationContext, setProgrammingModel } from '@azure/functions-core'; |
There was a problem hiding this comment.
Same question here about imports
| import { InvocationModel } from './InvocationModel'; | ||
|
|
||
| class ProgrammingModel implements coreTypes.ProgrammingModel { | ||
| name = '@azure/functions'; |
There was a problem hiding this comment.
Is the name of the programming model the same as the name of the npm package?
There was a problem hiding this comment.
Yeah, although it doesn't really matter. I'm just using the name here for tracking purposes in telemetry
| userLogCallback: UserLogCallback | ||
| request: RpcInvocationRequest, | ||
| userLogCallback: UserLogCallback, | ||
| doneEmitter: EventEmitter |
There was a problem hiding this comment.
For the most part, this function seem to be copy-pasted from the old worker. The only difference I see here is that, before, doneEmitter was created by this function, whereas now it's passed to it. Curious what difference it makes and why this decision was made?
There was a problem hiding this comment.
I needed to share doneEmitter between the InvocationModel methods getArguments, invokeFunction, and getResponse and it was easiest to create it on the class and pass it in here. Suffice to say it was just a byproduct of how the new code is structured and not some sort of grand design decision
|
|
||
| import Module = require('module'); | ||
|
|
||
| export function setupTestCoreApi(): void { |
There was a problem hiding this comment.
Not sure if I'm following this correctly, but it seems like here we're defining our own mock core API for testing purposes, which isn't necessarily the same core API provided by the worker. This means that if, for whatever reason, this API changes from the worker, we have to update it here too. I don't have ideas off the top of my head, but is there any way we can test against the actual core API directly, or link the two somehow?
There was a problem hiding this comment.
First of all, we don't have to mock the entire core api, just a few trivial enums. So I'm not worried about the engineering upkeep of the api changing
And yes there is a way to link the two - you'd have to essentially run the func cli as a part of the test. However that's more of an "end-to-end" test and even if we do that (which I want to do, just not sure when) it would not replace these unit tests. End-to-end tests take a lot longer to run and usually integrate external components. Unit tests should be super fast/simple and not rely on external components
Related to Azure/azure-functions-nodejs-worker#568
Here's the PR for the worker side: Azure/azure-functions-nodejs-worker#608
The most important file to look at is
src/InvocationModel.ts