Improving the exposed type system

[aaronpowell]

This PR shows how the generics can be used to make the exposed type interface more consumable to the end user.

There is one main issue, when using yield it is not possible to have the type inferred due to a limitation in the TypeScript type system (see microsoft/TypeScript#36967).

An option could be to change the Task class to inherit from Generator<T, T, T> and thus have a next method exposed so if people don't wish to use yield, and instead handle the iterations themselves, they can do that, which would give them type safety.

[davidmrdavid]

I have some preliminary questions about the changes :)

[davidmrdavid]

Great stuff! So, to summarize, this PR improves the type inference around entities which we previously short-circuited via any type annotations. Before I continue, I should say that I'm only just getting started with TypeScript, although I do have experience with type-systems in languages like Haskell, so the syntax here is not immediately obvious to me 😅 .

Am I right in understanding that these changes mostly affect the experience around declaring and implementing entities, but does not provide better inference for an entity client? This being because the entity client communicates with entities via yield-able calls which ,as you point out, are still a challenge to type-annotate.

So what is the current solution for typing yield-ed activity call? Do we just cast it forcibly via <yieldable> as <type> ?

Finally, I really like your idea of exposing a type-safe alternative API for activities. We could definitively expose an iterator, or alternatively use a continuation-passing-style / Promise / monadic syntax. @ConnorMcMahon , I seem to recall that y'all considered promises as a way to schedule activity calls? Any thoughts on revisiting that idea as an alternative, type-safe, variant for TypeScript? Just as an idea..., not a priority.

[davidmrdavid]

Ah, one final question for both of y'all.

Other than backwards compatibility concerns, these changes should be directly applicable to orchestrators, right?

[aaronpowell]

Yep, mostly this is around improving consumption of entity functions, and defining them by exposing more of the underlying types (that were previously any) or by giving the option to provide a generic argument to operations.

I want to get some time to explore moving where the generic argument is to higher up the definition chain, ideally when you define the entity function:

module.exports = df.entity<MyType>(function (context) {

That would be nicer if the MyType can flow into the context.df.getState/etc. functions.

[aaronpowell]

By and large they should be non-breaking operations, the only breaks would be when people are relying on the unknown or any types being returned. From what I understand, I haven't refactored any of the actual functionality.

[davidmrdavid]

Ah yes, having an entity declaration-level generic would be really elegant, definitely keep us posted! Let us know when you think you've done enough experimentation and we can decide if we're ready to merge and if any follow-up tasks fall from here: e.g, typescript entity samples. Also, at that point, we can probably discuss if it makes sense to port these directly into the orchestrator API.

And yeah, I don't think you've changed any functionality, we should be good! As always, many thanks, this is super helpful.

[aaronpowell]

I've got it working now with strongly typed entities, check out https://github.com/Azure/azure-functions-durable-js/pull/195/files#diff-b5b5544459f11b191866dffca7ebdac8 for a reference example.

Now what happens is that the entity function in shim.ts takes a generic argument (but defaults to unknown so it's entity<T = unknown>) and then the IEntityFunctionContext and DurableEntityContext are both generic, using that same generic argument. This then cascades through the generic on Entity and the weird casting stuff no longer is needed when creating the DurableEntityContext!

[aaronpowell]

And for funsies sake, I've also included generic orchestrators:

import * as df from "durable-functions";

module.exports = df.orchestrator<number>(function* (context) {
    const entityId = new df.EntityId("CounterEntityTypeScript", "myCounterTS");

    const currentValue: number = yield context.df.callEntity(entityId, "get");
    if (currentValue < 10) {
        yield context.df.callEntity(entityId, "add", 1);
    }
});

Now, you still have the problem that when you yield the result the yielded value doesn't know what type it is, but you get type safety if you try and pass in the third argument of callEntity. Although, I'm not sure if that's right, because the data you're passing into the entity doesn't have to match the type of the overall orchestrator...

[aaronpowell]

I see the entity's input and state are still coupled as the same type

Not quite @davidmrdavid, what I've done is defined that there is a new type for the input, TInput (same with return and TReturn) but the default type is TState which is the entity state.

This means that if you're working with simple types for the entity and input, you don't have to explicitly type it on input or return value.

[davidmrdavid]

It's got my LGTM! Sorry for the long delay here. Let me just make sure @ConnorMcMahon is also on-board and then we'll merge this

[aaronpowell]

All good, I'd totally forgot it was open 😅

[ConnorMcMahon]

I like the overall idea, especially for enforcing the state type for entities, but I think the generic methods on the orchestrator to specify input types will lead to some confusion, and I'm not convinced it provides that much value currently.

[ConnorMcMahon]

I'm a little concerned by the confusion this will cause in comparison with C#, where CallActivityAsync<T> has T refer to the return type, not the input type.

Unfortunately, with our use of generator functions, we don't have a way to cleanly specify a type for the return value.

[aaronpowell]

I'll admit I haven't looked at the C#/.NET API for a long time, and wasn't aware of how the type system there was represented (I didn't realise that input is object there, I assumed it would've been typed).

Admittedly, a lot of this PR was originally written around the Entities site of durable functions, not orchestrators, as I was trying to get them strongly typed so you can do stuff like I demo in https://github.com/Azure/azure-functions-durable-js/pull/195/files/8dc1bf93ebfda83db5ee8bda88764ba18e09e371#diff-efcc2398105c3d67a41a35488e1f984e83a7d96923c24c07bb9fa4e84a2c3fb6

Then I figured I'd apply the same pattern on orchestrators, came across the limitation with yield and went "eh, it's not breaking anything" so left it in.

[ConnorMcMahon]

I think orchestrations are a bit of a mess right now typing wise, and until we find a way around our current yield typing, I think it may be best to leave them as is.

I think the entity stuff we should definitely move forward with.

I'm willing to budge on the orchestration stuff if it feels sufficiently "typescripty", but I personally feel the cons of the generic typing outway the pros.

That being said, it still makes perfect sense for GetInput I would say!

[ConnorMcMahon]

I'm not sure I see the benefit of using generic parameters for Task unless we can enforce yield on Task<T> returns a value of T.

[aaronpowell]

Yeah, as I mentioned up top, the use of yield leaves you stuck with type inference unless microsoft/TypeScript#36967 is resolved. The only alternative would be to replace the usage of Task with a Promise and move away from generator functions to Promise-based async, but that'd be a rather huge change that I probably wouldn't advise without a major version bump and breaking changes 😛

[ConnorMcMahon]

100% agree. It would be really nice to get better type safety, but it's already somewhat user-enforced in C# even due to the fact that activities and suborchestrations are referenced by name instead of by type.

[davidmrdavid]

So it sounds like the fastest path towards merge is to remove the generic typing of callActivity and other orchestrator-specific APIs because it contradicts the typing in the .NET API (we type output instead of input). The only potential exception to this being getInput, which might be nice to type. Said differently, let's keep the typing in entities, remove it for orchestrators.

Is that right, @ConnorMcMahon?

[davidmrdavid]

Ok then @aaronpowell, it sounds like we have a clear path to merge then (outlined in my response above). Since it's been some time since we were last actively working on this, I could merge this into a temporary branch that I own and perform that refactor myself. Alternatively, if you have the cycles, you could take it over the finish line and refactor it yourself. I just want to be mindful of your time, especially considering how long we've kept this in PR-limbo. What would work best for you?

[davidmrdavid]

@aaronpowell, just a friendly ping about my question above^

[aaronpowell]

Sorry, been busy the last week. I'll try and get it tackled before the end of this week

[aaronpowell]

Rollback done.

[davidmrdavid]

The orchestrator-specific bits seem to have been roll'ed back. This seems good to me, let's hear from @ConnorMcMahon first and, if he agrees, let's merge this!

[ConnorMcMahon]

LGTM, just want to verify the @hidden removal to see if that was intentional.

[davidmrdavid]

Finally merged! Thank you all so much for the patience here, very excited to ship this! 🚀

[aaronpowell]