Split programming model out of worker #608
Conversation
There was a problem hiding this comment.
Overall, I think I follow what the code is doing, and no major issues there, but I'm not sure what goal this achieves?
As far as I understand, this so far only includes modifying function invocation, not function loading and indexing, which was the major difference and advantage of having new programming models. Also, I'm not sure what benefit allowing programming models to define invokeFunction() and getResponse() achieves? For example, what's an example of a programming model that's meaningfully different from the default one that would leverage those methods?
package.json
Outdated
| @@ -5,13 +5,12 @@ | |||
| "description": "Microsoft Azure Functions NodeJS Worker", | |||
| "license": "(MIT OR Apache-2.0)", | |||
| "dependencies": { | |||
| "@azure/functions": "file:../js-framework/azure-functions-3.4.0.tgz", | |||
There was a problem hiding this comment.
Shouldn't we import this as the npm package?
| } | ||
| isDone = true; | ||
| inputs = preInvocContext.inputs; | ||
| callback = preInvocContext.functionCallback; |
There was a problem hiding this comment.
Not also overriding the context here?
There was a problem hiding this comment.
That is correct. Similar to the debate we had for hooksData (#601), I don't want people overriding context, just modifying it. It is a class we as the worker created, and it has too much important information shared across our worker, the library package, and the user's app to be overwritten.
By contrast, the inputs and callback were created directly by the user and the programming model so I think it makes sense to let them overwrite it. Also they are just much simpler than context
There was a problem hiding this comment.
What do you mean? I'm talking about the plain context, which is created by the invocation model, not the coreCtx. This is one is created and managed by the programming model, so it makes sense if it wants to override it? How is it used by the worker?
There was a problem hiding this comment.
Yes you are correct it is the plain context not coreCtx so the worker does not create or use it directly. However, my larger point is still accurate in the sense that it has too much important information and should not be overwritten. The hooks are not a part of the programming model, so it would not be a case of the programming model overriding its own context.
|
This work is an important stepping stone on the way to the new indexing stuff. At its core, it is purely engineering work with no functional difference to the user. Perhaps that's why the "goal" isn't clear, but I did it that way because I think that makes it a lot easier to review. I try to separate "engineering PRs that affect a lot of files" from "feature PRs". The indexing stuff is tracked by #569 and it will be a separate PR - that PR will be a lot smaller (comparatively 😅), but it will also be a lot more complex because it's introducing new features. Please read through #568 if you haven't already. I think it answers a lot of your questions. I think the biggest piece you're missing is the type of code we want in each place. We want simple code with no chance of bugs in the worker because it takes so long to ship. We want more complex code (meaning more helpful code, but also code with a higher chance of bugs) in the npm package. Also keep in mind that people can build off of our programming model npm package. So even though I'm moving FunctionInfo (and other classes) into the npm package, people can still share that code and build off of it. In other words, a "new" programming model doesn't have to start from scratch - it can just wrap the one we provide in the npm package. |
|
Sent a meeting invite to discuss offline - I think that'll be easiest |
ejizba
force-pushed
the
ej/split-worker
branch
from
e3856e0
to
7b16140
Compare
The goal is essentially to split our code into two repos, while maintaining existing behavior:
@azure/functionsnpm package, which will include the entire programming model moving forward (not just the types, although it'll still have that). See PR here: Split programming model out of worker (library side) azure-functions-nodejs-library#1Most important files to review:
types-core/index.d.ts: This defines the API between the two piecessrc/eventHandlers/InvocationHandler.ts: This is the main file that uses the new apiFixes #568