fix middleware/context model about botbuilder-js HOT 51 CLOSED

Can I offer another point of view:

On top of Bill's ask, I'd also remind everyone that we are likely going to see more bots calling each other, where "calling" could be anything from an entire round trip server/client, passing by two bots sharing the same process, all the way to a master bot operating just as a router for requests to different "sub-bots" as modules (we've seen them already).

With that in mind, you almost want to keep people away from the context, just like you want to keep people away from an ASP.Net Session/Application object (in the early days everyone over used these and got hurt for doing so eventually when applications became larger/more complex).

I'm no specialist in Express but unless you are doing anything network call specific (that directly relates to overriding/changing the http request/response), you probably don't want to encourage developers to decorate the http request just to pass variables along different function calls, that sounds like an anti-pattern and this is what is happening here a lot among bot developers. They are not doing it because they are trying to extend the middleware, they are doing it in their bots, to write simple bot logic and carry variables. And even if that is really how the world works in Express, I'd suggest that is a less relevant pattern compared to the patterns bot developers are trying to build?

So the point is that context isn't a contract nor a business rules engine and it shouldn't be used as such. If we are seeing developers using context objects that way, something seems off, because this model won't allow the scenarios above without a lot of hidden magic. In other words: This makes modularizing/reusing things harder. The more we encourage a clear in/out contract with well defined schemas, the more we allow graph-like scenarios here.

Sorry for jumping in, this discussion is fun :)

from botbuilder-js.

@billba thank you for the hug afterwards... By far my favorite part :)

I've updated the proposal in issue #66 to show a trimmed down TurnContext object to reflect a number of changes being proposed across several issues. I thinks this reflects the spirit of Bills proposal with regards to the shape of the core context object so please feel free to comment on the proposed shape.

from botbuilder-js.

Things that bother me about current approach of using context as a state bag...

• inability to type the context object
• likelihood of naming conflicts

Things I do like about this proposal...

• layering approach in general (progressive abstraction layers)
• extremely lightweight turn object at the bottom of the stack
• philosophy of injecting dependencies into middleware (testability, debuggability, avoiding naming conflicts)

from botbuilder-js.

So I'd like to break the discussion into a few separate parts:

• What's the shape of the context/turn object you get out of the box from the botbuilder package.
• How should our 1st party middleware approach adding functionality on top of the context/turn object.
• What should our recommendation be for 3rd party middleware with regards to extending the context/turn object.
• How should we approach typing the context/turn for a transpiler like TypeScript.

What's the base shape of the context/turn object

I've proposed internally that we should strip the base context object down to its bare bones in M2. There's a little bit more that I think should be on the context then what's proposed here but not much more. So I would say that I agree with this part of the proposal in spirit.

What I'm not sure I'm convinced of is the need to rename context to turn. We could leave it called context and you could wrap it with something called a turn. If you believe that turn is more descriptive then context, and I'm not saying it isn't, then having the thing the developer is going to actually work with called a turn seems desirable.

Should our 1st party middleware extend the context object?

The whole architecture of our current plugin system started with Express.js as its inspiration. Express has 10's if not 100's of thousands of developers, the majority of which are plugin consumers and some of which are plugin producers. I know @billba is probably tired of hearing me say this but I still believe we should generally just do things the way Express does. Why? Because its proved that it works.

In Express, out of the box you get HTTP request/response objects that have a basic set of members as defined by Nodes HTTP module. Everything you technically need to handle a request is already there but its a bit cumbersome to work with so Express provides some pre-built 1st part plugins that you add things to your processing pipeline like a bodyparser() which do the crappy bit of pulling down the stream for the body of the request and then potentially even parsing that into a JSON object which it tacks on to the request object as property called body. There are a few dozen of these 1st party plugins that do generally useful things in the relm of HTTP. Some extend the context object and some don't.

You could totally envision a version of bodyparser() that doesn't extend the request object and instead caches it internally and has you pass around the bodyparser() to every component but that's not the way it works in Express. I'd argue that it doesn't work that way because the simplest thing to do is just tack it onto the request object. It's the parsed body for that request and you want it to live for the lifetime of the request so why shove it into a separate cache that you know have to manage the eviction of? It's easier to just tack it on the body.

I'd argue that we have the same needs... We have context/turn object that lives for the lifetime of a single request. We could totally call out to LUIS, get the intents for the request, and then shove them into an external cache that we then have to manually evict once the request is done but why? It's easier to just tack it onto the context/turn object so that it will get automatically cleaned up when that object goes out of scope. Maintaining multiple other caches that you then need to explicitly bind to the lifetime of the request object just seems like unnecessary work. Yes we should ensure that when we save something like an intent to the context object it doesn't clobber other intents but that's a completely separate problem.

What about 3rd party middleware extending the context object?

If you read Expresses guide for writing middleware they briefly mention that you can change the request and response objects but that don't dwell on the topic or even give any guidance for how to extend those object. I believe that's because the vast majority of 3rd party middleware doesn't have a need to extend those objects. They just layer themselves on top of the extensions made by the 1st party middleware and they'll say "you'll need to make sure you have used the bodyparser() before my middleware". Yes it means that dependencies are expressed using documentation but this doesn't generally seem to be a problem.

The other thing is... Even when you have a piece of 3rd party middleware that mucks with the request/response objects, there are social conventions preventing one piece of middleware from clobbering another piece of middlewares extensions. Nothing technically prevents me from writing a piece of middleware that overwrites request.body with whatever I want but I'm not going to create a piece of middleware that does that because a) no one would use my middleware because it would break their website, and/or b) everyone would yell at me and tell me to use a different property because I broke their website.

I think we could generally give the guidance that you should avoid adding stuff onto the context object unless you absolutely have to. But I'm honestly not sure how prescriptive we need to be here as I don't think most people will even have a need to. I could definitely wrong about that but until we get a bunch of people actually building useful middleware we don't know and I just want to make sure that we're not trying to solve a problem that we don't really have.

What about issues around correctly typing the context object?

Lets start from a very clear place that this is a TypeScript specific issue. If you're using vanilla JavaScript or something like Babel this isn't an issue you have. That doesn't mean TypeScript isn't important I'm just saying that it affects a subset of developers using the SDK.

Starting from there I think we can approach solving the typing issues in a number of different ways. I don't like the ambient interfaces anymore than anyone else does. I hate them in fact. We could potentially use external typings with interface merging so that the developer can precisely describe the shape of the context object they're using. Or if that's not great we can approach it from some other direction. The thing I want us to not do, is have our desire to properly type everything heavily influence the architecture of the SDK. I love TypeScript as much as everyone else on here does but this is the JavaScript version of the SDK not the TypeScript version.

from botbuilder-js.

Now with annotations, this can be solved fairly easily, albeit at the cost of a small performance hit. For example, lets say we had this:

class MyCoolContext extends BotContext {

 public someGreatFunctionality() {
    const {conversationReference, state} = this; // superclass members
    // Do something awesome! ...and custom! 
  }
}

class AnotherCoolContextType extends BotContext {
  public aGreatFunction() {
     const {conversationReference, state} = this; // superclass members
    // Wee!
  }
}

class MyCoolMiddleware implements Middleware {
  // No annotation, send it a regular BotContext
  public contextCreated(context: BotContext, next: () => Promise<void>): Promise<void> {
    // No annotation, a regular old BotContext
  }
  @Context(MyCoolContext) // Specify this method wants the MyCoolContext type
  public receiveActivity(context: MyCoolContext, next: () => Promise<void>): Promise<void> {
    // Wow! typed context! Sawweeeet!
  }
  @Context(AnotherCoolContextType ) // Specify this method wants the AnotherCoolContextType type
  public postActivity(context: AnotherCoolContextType /* rest of the arg signature */ ) {
    // stupid cool stuff happening here!
  }
}

We can use decorators to specify the context type we want to receive in each middleware method. The MIddleWareSet would then maintain a Map of the typed BotContext subclass instances and match them up with the annotated method signatures.

Each instance of the BotContext, subclassed or not would receive a shared library of property values that are proxied to all instances. This would allow us to maintain symmetry so that changing the conversation object on and instance of MyCoolContext propagates to the instance of AnotherCoolContextType . for example:

const commonBotContextValues = {}; // Populated at each turn will common values
const instance = new Proxy(new AnotherCoolContextType(), {

  get: function(target: BotContext, prop: PropertyKey, receiver: ProxyConstructor):any{
    if (prop in commonBotContextValues) { // A common property shared across all botContext types
        return commonBotContextValues[prop]
       } else {
        return target[prop]
     }
  } 
});

Now, no matter which instance of the subclassed bot context is calling for the state property, it will always be the same state object instance. Also, this automatically gives us a revocable context.

from botbuilder-js.

So @tomlm and I were discussing this subject and beyond the broader discussion about middleware & context there were two low hanging changes we could make that seemed worth capturing. The last one, #65, I think I ended up convincing myself that you can already easily compose a 'proactive' activity that gets routed (its 3 lines of code) so I'm not sure there's any work to actually do for that one.

from botbuilder-js.

A couple of notes:

• Before discussing the solution we should decide if we agree that there are problems. There's no use discussing solutions if we don't agree on the problems.
• In many ways the JavaScript repo is the worst possible place to have this conversation, as JavaScript's flexibility softens some of the problems underlying architectural messiness.
• I think a good test of our model is that there is no difference between how 1st and 3rd party middleware is authored and treated by the system.

from botbuilder-js.

Suggestion.
We already have a conventional state object that hangs on context. This is the intended home of everything the bot developer needs to persist. How about we also hang on context (again by convention) a transientState or turnState object and then offer the guidance to bot and middleware developers that "your stuff" should go in one or the other depending on whether you want to persist it. We did something much like this in the Windows Runtime API.
This puts the burdens (including persistence, type management, etc.) entirely on the developer, and allows a BotContext type to (likely) remain as is.
Middleware writing directly to context wouldn't be disallowed, it would just be considered poor form. Middleware developers may even be encouraged to accept an optional object where data should be recorded - a developer could pass in context.state or context.transientState as a dependency and then know exactly where his topIntents are going to be.
Disclaimer: sometimes I have bad ideas and I'm not afraid to hear it.

from botbuilder-js.

@codefoster that was actually my original intention with context.state. It's your turn state and anything you append there won't get persisted. In hindsight, the fact I hung context.state.conversation and context.state.user of that and those DO get persisted I think confused things. There's definitely value in having scratch memory that lives for a single turn and we should separate those out.

I personally still don't think there's a one size fits all solution here. I'd really like to see us start with just stripping everything off the context object and getting it down to its essence. That way I can have 3 different frameworks or libraries that approach layering on top of that in 3 different ways. The proposal in #66 roughly mirrors the turn object @billba proposed and gets the context down to just the bare essentials needed to support the Bot Framework Protocol.

From there we should take each concept like state storage, intent recognition, analytics, etc. and figure out what's the best way those should layer on. If that results in a single approach then great.

from botbuilder-js.

@billba We could move the discussion to the .net repo but I think moving it to the most restrictive one, java, would result in no one seeing the updates to the discussion. Agree that JavaScript has the most flexibility approach wise so would like to avoid just designing for JS/TS.

I think whatever we come up with will work the same for both 1st party and 3rd party plugins but that doesn't mean you can't give slightly different guidance to 3rd party developers.

As for whether or not we have a problem or not. I definitely believe that our current 1st party middleware implementations are broken and we should revisit every plugin. I'm not convinced the middleware stack itself is broken. I say that because I've built some pretty sophisticated middleware and I have yet to run into something I wasn't able to do. Going a step further, since moving to the russian doll approach there's nothing that even feels that brittle in the implementation. I'd also say that we can speculate about what would be a better plugin model all we want but until we get a bunch of people writing real world plugins we're not going to know if what we have is broken or not. It doesn't currently feel broken to me but that's just one opinion.

from botbuilder-js.

@Stevenic it is worrisome to me that your interpretation of my problem statement is "middleware can't do everything we want it to." The problem is not that it's not powerful enough - the problem is that injecting functionality directly into a single object doesn't scale well to a large, diverse ecosystem and (to @matvelloso's point) large, complex bots.

(Also, @Stevenic I'm not really proposing moving the conversation to a different repo. I'm just saying we should be having the largest version of this discussion, not focus on JS.)

from botbuilder-js.

I totally agree that this discussion has to be from the perspective of a language like Java, where we can't just do dynamic foo, and we can't patch things up with clever use of extension methods.

I am perhaps naive, but from my perspective Middleware should ONLY be exposing an interface, the context object itself should not be dynanmic expect through interface

So what is the way the pattern we set up to do this? To me, it looks like either this (for generic languages)

• context.Set<IFoo>(intance) or
• context.Get<IFoo>()

If you have a language which has no generics support it looks like this:

• context.Set("Mynamespace.IFoo", value)
• context.Get("Mynamespace.IFoo")

The point being that there is not namespace collisions because you set up the pattern that the only way you can extend the context object is through namespace interface.
In the generics languages the namespace is built in, in the non-generic languages the samples all use namespace qualified names.

For the consumer it looks a lot like Bill's code and require() or using() patterns

   Bot()
     .Use(MyFooImplementation())
     .OnReceive(ctx => {
         IFoo foo = ctx.Get<IFoo>();
         if (foo.someProperty)
             foo.SomeMethod();

      });

Doesn't that meet the requirements of issue we started with?
-Tom

from botbuilder-js.

To make this more concrete with IStateManager

Bot()
    .Use(StateManager())
    .OnReceive(ctx => {
        IStateManager state = ctx.Get<IStateManager>();
        if (state.conversation.greetedUser == false) {
               ctx.reply("yo");
               state.conversation.greetedUser = true;
          }
     });

On Java:

Bot()
     .Use(StateManager())
     .OnReceive(ctx => {
         IStateManager state = IStateManager.Get(ctx);
         if (state.conversation.greetedUser == false) {
                ctx.reply("yo");
                state.conversation.greetedUser = true;
           }
      });

from botbuilder-js.

My understanding of the problem is that we need a way to use middleware in a way that scales without polluting the context object. I'd like to understand why a DI model was ruled out. I'm sure someone thought of it at some point in this project. DI is supported by most all languages in one way or another (including JS with my example above). for example, It seems reasonable to have:

@Injectable()
class MemoryStorage extends StorageMiddleware<StorageSettings> implements Storage {
  // rest of class here
}

And inject it...

@Injectable()
class class BotStateManager implements Middleware {

  @Inject(MemoryStorage)
  public postActivity(ctx: BotContext, memoryStorage:MemoryStorage) { 
    //... so stuff
  }

// rest of class here
}

Then use it in a concrete implementation of your bot.

@Use(MemoryStorage)
@Use(BotStateManager)
class MyBot extends Bot {
  constructor() {
    this.onReceive(this.receiver);
  }
  
   @Inject([MemoryStorage, BotStateManager])
  private receiver(ctx: BotContext, memoryStorage:MemoryStorage, stateManager:BotStateManager) {
   // call the middleware instance directly
  }
}

const bot = new MyBot();
// etc...

This approach would work in all languages supporting a DI container, scales well, seems to provide good encapsulation and separates concerns while giving an explicit definition of what's to be expected within the method signature.

from botbuilder-js.

from botbuilder-js.

I would rule out dependency injection for a few reasons:

1-The use of dependency injection or not should be a decision of the developer using the framework, not the framework itself. So whatever solution we come up with, it should totally let the developer take their DI framework of choice, plug it in, and do whatever they want. But never be an opinionated framework that forces them down their path

2-In the past, the C# framework made exactly that decision of choosing a favorite DI and samples were using it all over the place, this became a key source of issues across developers.

So before going with DI as a solution there is a core level decision here to be made and that decision is independent to DI frameworks.

from botbuilder-js.

@matvelloso - what if we can accomplish this without a framework? Java does not require a framework for DI, nor does JavaScript. Most languages can either use reflection or a decorator pattern to perform these responsibilities.

from botbuilder-js.

Also, why not leave this middleware problem upto the developer? It should be fairly straightforward to solve in a subclass that extends Bot by simply overriding the interface functions and conditionally passing arguments. e.g.

// Oversimplification example
class MyBot extends Bot {

  public receiveActivity(context: BotContext, next: () => Promise<void>): Promise<void> {
    this.middleware.forEach(middleware => {
       if ('receiveActivity' in middleware) {
            if (middleware instanceof MySpecialMiddlware) {
               (middleware as MySpecialMiddlware).receiveActivity(context, aSpecialObject);
            } else if (middleware instanceof MyOtherMiddleware) {
               (middleware as MyOtherMiddleware).receiveActivity(context, anotherSpecialObject);
         }
    });
  }

}

from botbuilder-js.

If you do DI without a DI framework, you will be effectively writing your own "DI framework" anyways, whether it is just a half of a framework or a full one, baked into the bot framework. Either way, you are forcing the developers into a pattern they might just not be in agreement with. Except now they might have to live with their DI framework + yours. Again, I think we are jumping into a much lower level discussion that isn't really where the problem is. DI is just one of the thousands of ways developers could get around this, (and if they want it, by all means let them do it).

Re your second question: Why not leave this middleware problems up to developers? That is exactly what we want. The only "gotcha" is that they will likely follow the patterns we suggest them in our libraries, samples and documentations. Which is the point Bill and Tom discussed earlier on, about being mindful on what path we suggest them to take.

from botbuilder-js.

Yes, all I'm actually proposing is that we get rid of the createContext middleware. Everything else in my strawman is just a demo of how one might package services to make them available to middleware and other bot logic.

from botbuilder-js.

@tomlm your Java example looks almost identical to my strawman! It's simple and clean and perfect! Do that in JavaScript and C# too!

from botbuilder-js.

Adding BotContext.get() and BotContext.set() is also what I proposed in a DCR a few weeks back so I'm totally on board with that. I'm not showing that on the trimmed down context object I proposed in #66 but that simply because I was trying to get the context down to its non-controversial basics first. I would be happy to see those added in as I agree that pretty much every language should support get() & set() and then if you need to store something for book keeping purposes if nothing else you can use a GUID to avoid collisions.

In proposal #65 I believe I mentioned that I was trying to decide if we could eliminate the contextCreated pipe and just always run receiveActivity for both the proactive and reactive cases. I think I'm personally still leaning towards the need for both but I could potentially be shifted towards consolidating the two into a single 'receiveActivity' pipe. I just want to be clear about the trade offs of combining those pipes.

Currently, when an activity comes in it flows through both pipes in order. Whats nice about that is that any dependencies can be added to the context in the first pass ('contextCreated') and those dependencies can be consumed in the second pass (receiveActivity). Dependencies can take several forms and it could be something like a call to LUIS that then wants to add the recognized intent to the context object or something loading the conversation state for the turn. Using state loading as an example, if you load the turns conversation during the first phase then potential consumers of that state running in the second phase don't have to care where they're at in the stack. Plugins are generally less sensitive to their position in the stack. If we combine the two pipes into one plugins will be generally more sensitive to their position in the stack.

Proposal #65 suggests that we move to a model for proactive messages where a proactive message isn't that dissimilar to a reactive one. You run the same reactive pipe(s) but with an activity of type 'proactive' instead of 'message'. That's in itself is an interesting simplification but it does mean that things like your LUIS recognizer now need to be aware of the proactive case and should only call LUIS for reactive messages where the intent is unknown. This could just shake itself out naturally given that proactive messages won't have a text field. I think I'm generally ok with this simplification and not too worried about the small added burden. There's another related tangent around human hand off but I'm not too worried about that at this point either.

So for me the discussion around whether we can just combine the contextCreated and receiveActivity pipes comes down to if we're ok with increased order sensitivity by not having two passes through teh middleware stack?

from botbuilder-js.

The big question is not whether we should combine contextCreated and receiveActivity. The big question is, should middleware inject functionality into a single object?

I claim that this is ultimately a brittle pattern, no matter how you do it. Even if you get/set with namespaces you are still creating an invisible web of dependencies among middleware, hard to test.

My strawman and Tom's Java example illustrates that you don't need to do this.

from botbuilder-js.

If we think of the context object as just a convenient cache that lives for the lifetime of a turn then I think passing the context object into some static helper is totally fine. In v3 we had Prompts.text(context, "what's your name") which took the same approach. The one thing I'll say, and this is more of a JavaScript thing, is that developers consistently forget to pass in the context object so you just need to protect against that.

The other pattern I've seen referenced is the const myExt = new MyExtension(context); which we also have in v3 with const msg = new Message(context); and the feedback has clearly been that developers hate it.

from botbuilder-js.

No, I do not think we should think of the context object as "just a convenient cache that lives for the lifetime of a turn." I think it should be a primitive, unopinionated, immutable object created per turn.

Then, per turn, after the middleware runs, the developer can take this object and use it to create their own context object, with all the functionality they want, e.g. context.message, which would really just map to new Message(context).

from botbuilder-js.

Building on @billba's and @tomlm's idea, It might be possible to abstract middleware responsibilities into a command based interface. This way, the MiddlewareSet does not need to know anything about the implementation details or about the object type, it simply executes a command and returns a promise. This would obviate the need to call methods on the object directly. Each time a command is executed, it would be transient, stateless and fully encapsulated. The command would then have the burden of performing the work or orchestrating other complex interactions within the other tiers of the application asynchronously. The addition of a Command interface and a registration mechanism would be the only new dependency.

e.g.

interface Command {
  execute(cxt: BotContext): Promise<any>;
}

Then register the command(s):

new Bot()
              .registerCommand('getState', GetStateCommand) // Encapsulates all logic for getting the state
              .registerCommand('greetUser' GreetUserCommand) //  Encapsulates all logic for greeting
              .registerCommand('promptForName', PromptForNameCommand) // etc....
              .onReceive(async (ctx: BotContext) => {
                  const state = await ctx.execute('getState');
                    if (!state.conversation.greetedUser) {
                       const userGreeted = await ctx.execute('greetUser');
                     } else {
                       return ctx.execute('promptForName');
                      }
               });

The Bot framework is now simplified a bit more and a the API is reduced to a simple interface. This could be repeated for each method in the Middleware interface. Also, this allows the developer the flexibility to decide overall application architecture.

from botbuilder-js.

I see... There's obviously nothing we can do to enforce immutability in most cases but I'm not philosophically opposed to approaching it from that angle. I guess it just all depends how cumbersome it is to assemble the object layered above the primitive context object. You're using spread operators to do that in your sample but that's obviously not going to work in other languages.

To be clear... The thing that rubs me wrong at this point in the discussion is that it feels like to me our quest to find one single way of working with the context object that's the same across all languages is coming at the cost of us ignoring language features in some languages. For instance doing this in JavaScript:

Bot()
     .use(new StateManager())
     .onReceive(ctx => {
         IStateManager state = StateManager.get(ctx);
         if (state.conversation.greetedUser == false) {
                ctx.responses.push(MessageStyler.reply("yo"));
                state.conversation.greetedUser = true;
           }
      });

instead of this:

Bot()
     .use(new StateManager())
     .onReceive(ctx => {
         if (ctx.state.conversation.greetedUser == false) {
                ctx.reply("yo");
                ctx.state.conversation.greetedUser = true;
           }
      });

Simply because you want the context object to be completely immutable seems like you're just making things more complicated for me as the developer. Not less...

from botbuilder-js.

@Stevenic - I tend to agree that cross-language symmetry could lead to a slightly more generic or naive implementation. Abstraction via interfaces can help keep the clockworks language-specific.

from botbuilder-js.

We just banged on this a bit internally and I think we all generally like the move to context.get/set and we can certainly start from a position that the StateManager for instance will expose some helpers for working with state having the context passed in.

Going even further I definitely think it makes sense that the goal for any 1st party components we produce is they should be use able free standing outside of the scope of middleware which I know is something you've been pushing for as well @billba. So if I have a StateManager I should be able to say const state = new StateManager(context); and use it directly from my bot logic. It just may mean that you now have to explicitly call state.read() and state.write(). The advantage of using it as middleware is that the state will be tied to the lifetime of the context and the read() & write() will happen automatically.

from botbuilder-js.

I'm hoping the proposal does not include making me - a JavaScript developer - use .get() and .set() just because the Java language exists. I don't think anyone is saying that, but I just want to be sure.

from botbuilder-js.

@codefoster what would you have the JavaScript developer do? No matter what the StateManager() middleware needs to maintain some bookkeeping and state relative to the context object. This can either be done by tacking stuff onto the context or maintaining it in a separate cache which you now need to evict. Given that you want the state your caching to live for exactly as long as the context object does why wouldn't you just store it there? Maintaining a separate cache that you have to do work to keep in sync with the context object seems crazy to me... I'll refer back to my Express example of needing to call bodyparse(req) every single time you wanted something off the body of the request object. The bodyparser() just tacks the parsed body back onto the request object because it has exactly the right lifetime semantics and there's no need for a separate cache that needs to be evicted when the request goes out of scope.

from botbuilder-js.

One additional option I wanted to throw out for TypeScript with regards to the typing problem is you could use standard interface inheritance to describe the shape of the aggregated context object:

interface TurnContext extends BotContext, StateManager, ReplyMethods {

}

const bot = new Bot(adapter)
    .use(new StateManager())
    .use(new ReplyMethods())
    .onReceive((context: TurnContext) => {
        context.state.conversation.foo = 'bar';
        return context.reply('Hi!');
    });

This leverages the fact TypeScript lets you define an interface and a class with the same name. If you just define a top level TurnInterface that aggregates the interfaces of all the middleware the bot has used you can then type your context object to be the shape of the aggregate. Bonus is you'll actually get a compiler error if you added two pieces of middleware that try to declare the same property or method.

from botbuilder-js.

I only mean that it shouldn't have to literally look like state.get("mything") since JS let's you state.mything.

from botbuilder-js.

@codefoster was just about to clarify if that's what you're asking :)

I had been thinking about adding get(), set(), and has() methods but let me explain why. One issue with just tacking stuff onto the context object directly is that we're right back into the same "the context object should be immutable" situation we're discussing, adding the 3 methods at least helps with the immutability. A more important reason is that we'd want to throw a somewhat meaningful exception if you tried to get("foo") and "foo" hadn't been set(). The has("foo") method allows you to explicitly check for "foo" without an exception. We could work around the need for all this using a JavaScript Proxy object but that isn't supported in all browsers (curse you IE and Edge) and we want you to be able to write bots that run in the browser.

from botbuilder-js.

@Stevenic You asked "given that you want the state your caching to live for exactly as long as the context object does why wouldn't you just store it there?"

Literally for all the reasons stated here: https://github.com/billba/middleware#the-problem

No one is arguing that the current method isn't extremely straightforward and convenient in JavaScript. Just that it won't scale.

from botbuilder-js.

I totally agree that we need a solution to this problem in all the languages the SDK wants to support. I just disagree with a lowest common denominator solution. We should pick a strategy that's most appropriate for each language and ensure that the SDK's concepts are preserved across all languages. I personally don't think the way plugins expose their functionality to the bot has to be identical across every language. If they end up looking similar enough that you can look at the code and they look roughly the same then great but lets not do lowest common denominator please.

from botbuilder-js.

@justinwilaby

What you have here:

>               .registerCommand('getState', GetStateCommand) // Encapsulates all logic for getting the state
>               .registerCommand('greetUser' GreetUserCommand) //  Encapsulates all logic for greeting
>               .registerCommand('promptForName', PromptForNameCommand) // etc...

... sure seems very close to Redux Actions. It's a bit different, but the formal exercise of "Create a Function for each of your state transitions" ends up with very deliberate state manipulation.

Here you're not quite doing that, as "greetUser" is quite a bit more than just state management, but the pattern is clearly a good one.

I've seen (recently) bots build using the V4 C# SDK that are driving all of their prompts from a JSON file. In that they define the prompt name, text, and SSML. They gen, using T4, strong types so they get intellisense, and then the party begins.

from botbuilder-js.

@Stevenic I'm not proposing a lowest-common-denominator approach. I'm all for solutions which feel right in each language. I'm simply saying that the current JavaScript approach does not scale, for the reasons set forth here. I'm open to any solution which solves them.

from botbuilder-js.

I just read through redux's middleware guide and they mutate objects in their samples (see readyStatePromise example) and express middleware sometimes does as well so I'm not sure I agree with the doesn't scale comment. You do have to use common sense and you should avoid adding stuff unless it makes sense to do so.

I've written over a dozen pieces of middleware for v4 so far and only a couple have needed to add anything to the context object and even then its like a flag to communicate something from the receiveActivity to the postActivity handler and you can use a GUID for the key in that case to avoid any collisions. For everything else I use the same trick redux does which is to just hold things in the requests closure state.

I just keep pushing back on this because I honestly think it going to be rare that you need to store anything on the context object and even more rare that you'd add something like a function or property that you'd expect a developer to call. The vast majority of plugins will just use the things our 1st party middleware defines on the context.

from botbuilder-js.

Steve, Bill is raising a pattern that we have seen dozens of developers following. So it is a fact: It doesn't scale and will happen unless we suggest a different approach this time around. The fact that you wrote many pieces and didn't fall into that trap gives a data set of 1. Beyond your opinion, how many developers you saw going down a different path when building their bots on this framework?

from botbuilder-js.

@matvelloso my observation about the "doesn't scale" comment is that I don't see that we're doing anything that significantly different than what redux or express does with regards to middleware. Those frameworks combined have hundreds of thousands of developers who have built thousands of pieces of middleware. If our approach is basically the same as theirs then they should have the same scale issues yet given the numbers of devs using those frameworks I find that unlikely.

I'm honestly trying to find the trap(s) I'm not falling into. I've found a couple hence why I keep proposing improvements #74. And it's very likely I'm just not groking the points you're all trying to make. I'm happy for us to all get in a room and bang this out on a whiteboard. We all want to get this right.

from botbuilder-js.

Let me be more concrete about the dangers of the current approach, including the various mitigations you have proposed.

Missing Dependencies

Given:

bot
    .use(Y)
    .onReceive(myBotLogic)

Let's say Y depends on some other piece of middleware X having modified context. If I didn't read the docs (or the code), there's no way to know this. It is only discoverable at runtime. And if Y only uses this dependency in unusual circumstances, it may not show up in everyday testing.

Removing Dependencies

Given:

bot
    .use(A)
    .use(B)
    ...
    .use(Y)
    .use(Z)
    .onReceive(myBotLogic)

Is it safe to remove A? Again, my testing might not show the problem. I have to read the docs and/or code for B through Z, and of course myBotLogic.

Encouraging middleware to add things to context and/or rely on things added to context creates a fragile development environment. @Stevenic your argument is "this is an established pattern in JavaScript and the community has made it work." There's no question that this is obviously true. And ultimately we can't stop anyone from shooting themselves in the foot.

But that doesn't mean that we should encourage it or, worse, actually require it of the users of our middleware, some of whom, like me, will actively resent that they are forced to use inferior patterns. You are limiting my freedom, as a developer, to create a robust architecture.

I think we all agree that most developers will follow the lead we set with our first party middleware and samples. We should lead with the best possible practices, the ones that will set developers up for success.

from botbuilder-js.

@billba my concern is that the approach your proposing leads to code that looks like this:

import { dispatchActivity } from './activityDispatcher';

const logger = new MyLogger();
const state = new BotStateManager(new MemoryStorage());
const bot = new Bot(adapter)
    .use(new LogActivties(logger))
    .use(new StateReaderWriter(logger, state))
    .use(new DialogVersion(logger, state, 2.0, (ctx, ver, next) => { }))
    .onReceive((ctx) => {
        const turn = {...ctx, logger, state };
        return dispatchActivity(turn);
    });

versus this:

import { dispatchActivity } from './activityDispatcher';

const bot = new Bot(adapter)
    .use(new MyLogger())
    .use(new BotStateManager(new MemoryStorage()))
    .use(new DialogVersion(2.0, (ctx, ver, next) => { }))
    .onReceive((ctx) => {
        return dispatchActivity(ctx);
    });

It makes it nice for the developers bot logic because they get to build up a new context object that has the combined view of everything they care about. But it makes the signature for middleware components being added look horrid because they don't get that same benefit.

Even the act of creating the combined turn object works ok in JavaScript because you have spread operators but in other languages you're going to have to either wrap everything with a new Turn class that makes you do turn.context, turn.logger, turn.state, etc. or just pass the dependencies as individual params to everything.

I understand your concerns with the brittleness of the dependencies but my concern is the impact to the overall readability of your bots code as you could end up with middleware plugins that need 5+ arguments passed in.

from botbuilder-js.

It's actually even worse then that... Now... No component can assume that things like state have been read in yet so all of your code (bot included) turns into either a series of await statements (if your language supports that) or long promise chains. I get the purity of what you're proposing but it just feels like it comes with a high cost.

from botbuilder-js.

Yes, my repo is just a series of annotated samples that illustrate exactly these points. (Your examples swap my notions of turn and context, but otherwise you've got the idea.)

First and foremost, I think your eye is trained to the simplicity of the current context model. Please remember that, if we change things, most developers will not have seen the old model, and so they will not share your problem. They will see a plugin model where the constructor for each plugin takes an argument for each dependency. And that is a pretty normal thing, I think you will agree.

I will also point out that configuring the bot to use middleware is a thing that will typically happen just once, when a bot is created, and often just by copy/pasting boilerplate from another bot/sample. Virtually all a bot developer's time will take place in the bot logic, where their experience can be almost identical to the current experience.

In my proposal I point out that in other languages the creation of a context would most likely happen through a class. You can still "spread" the contents of one object into another, it's just a more manual process. Again, this typically is boilerplate code written once per bot. And, again, mostly it will be a copy/paste operation from other bots/samples.

If they prefer, the developer of a piece of middleware can also utilize a context, again mostly a copy/paste boilerplate operation.

This context creation function/class is the only place where the fetching of any async services needs to happen. The point of the withContext pattern is that the consumer of the context doesn't need to see any of this.

To summarize, the current context model is the epitome of simplicity. It simply doesn't scale. So we will have to pay the price of a little less simplicity. However I do think it's just a little less, and I do think the way I have structured my samples shows that, for the most part, the average bot developer can be insulated from virtually all of it.

(I updated my proposal slightly to emphasize some of these points)

from botbuilder-js.

FWIW, the group I was hacking with today (Flyreel) ran into just this issue. We shoved something in the context, TypeScript started screaming, we declared the thing with a declare global. The team suggested...

let myThing;
bot(...)
   .use(new MyMiddleware(myThing)
   .onReceive(context => {
      myThing.doMagic();
   })

The only shortcomings I see are...

1. the more verbose code on setup, but I agree that I'd rather have a little bit of complexity here than in the logic. Not to mention the ugly declare global if you go that route.
2. the fact that myThing can't be passed to other modules in the context object, but isn't it just fine to do something like this...

//app.ts
export myThing;
bot(...)
   .use(new MyMiddleware(myThing)
   .onReceive(context => {
      myThing.doMagic();
   })

//module1.ts
import { myThing } from './app'

myThing.doMagic()

from botbuilder-js.

@billba I'm sorry I should have just done a sample earlier in the thread to show that I totally get what you're proposing but here's the issues I'm wrestling with. I also wish I could do [cil] so I could line item comment on each point :) I generally don't disagree with what you're saying. I totally get that in your bots logic you can streamline things by ensuring that state's been read in and such and I'm potentially ok with this just being a heavier burden on middleware developers but what I'm striving for is something that feels well balanced between the middleware and bot developer.

@codefoster The global thing sucks and should go away. I think there are several approaches to improving on this part.

Turns out I'll be able to stay all day Friday so maybe a couple of us can grab a room with a whiteboard tomorrow afternoon and work through this.

from botbuilder-js.

I'm not remotely attached to the specifics of my proposal, however I also can't imagine how it could work significantly differently and still solve the underlying problem. I'm hoping you can pull a rabbit out of your hat.

from botbuilder-js.

After talking with @Stevenic in person we are a lot closer on this.

We are now in agreement that Turn (to use my terminology) can act as a cache for values. I have updated my repo, adding _get, _set, and _delete primitives to Turn, which allow any service to cache objects over the lifetime of a turn.

At first glance this looks the same as what we were doing before - adding functionality to context. But it's actually quite different. Instead of e.g. accessing context.state or context.get("state"), you would need to do e.g. stateManager.get(turn) - under the covers it would look in turn for a cached value by calling turn._get(key), where key is some calculated string, and if it's not there it would fetch the state from storage and put it in the cache using turn._set(key, state). In my example key is the name of the service, e.g. "Microsoft.StateManager" plus the turn.id. In fact the more obscured the key the better, to discourage developers from trying to grab it directly using turn._get.

This removes the ugliest part of my proposal - a separate turn cache.

P.S. @Stevenic good work pulling that rabbit out of your hat

from botbuilder-js.

THERE WERE HUGS INVOLVED AND I MISSED IT? oh man :(

from botbuilder-js.

Both SDKs implemented this change in full, and so I am closing this. Thanks to everyone who participated.

from botbuilder-js.