-
Notifications
You must be signed in to change notification settings - Fork 275
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
fix middleware/context model #56
Comments
Things that bother me about current approach of using context as a state bag...
Things I do like about this proposal...
|
So I'd like to break the discussion into a few separate parts:
What's the base shape of the context/turn objectI'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 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 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 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. |
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 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 |
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. |
A couple of notes:
|
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 :) |
Suggestion. |
@codefoster that was actually my original intention with 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. |
@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. |
@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.) |
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)
If you have a language which has no generics support it looks like this:
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. For the consumer it looks a lot like Bill's code and require() or using() patterns
Doesn't that meet the requirements of issue we started with? |
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;
}
}); |
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. |
Just to throw in another interesting curveball, BLIS needs to have access to both entity state (i.e. the set of entities that have been accumulated / set over the course of a conversation from use utterances) as well as the internal state of the bot, so that the NN can predict the next response to send.
We have a concept of “EntityMemory” in which both entities identified by LUIS and those set by the developer programmatically (e.g. isLoggedIn, isStoreOpen) are stored.
The Entity types are defined by the user BLIS bot editor and they are provided with a set of API calls for updating Entity state.
From: Justin Wilaby [mailto:notifications@github.com]
Sent: Monday, February 12, 2018 10:13 AM
To: Microsoft/botbuilder-js <botbuilder-js@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Subject: Re: [Microsoft/botbuilder-js] fix middleware/context model (#56)
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 though it at some point. 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.
class MyBot extends Bot {
constrcutor() {
this.use(MemoryStorage ());
this.use(new BotStateManager())
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.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2Fbotbuilder-js%2Fissues%2F56%23issuecomment-365011021&data=04%7C01%7CLars.Liden%40microsoft.com%7C3bc547eddeb5419d9d1f08d572444262%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636540559857377131%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=dZOI7J%2BYdqNDXVxRsI6DtSUv%2BXE7G%2BumU41xt1DQUJU%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAGOd5pa7elgLBhR2C-SIsLqzQOVXQ0bQks5tUH8rgaJpZM4SAH1L&data=04%7C01%7CLars.Liden%40microsoft.com%7C3bc547eddeb5419d9d1f08d572444262%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636540559857377131%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=1GZ424OTzZa%2B3Kp9UOaSoCN77zzKQ2XKvO%2BSP4bQGWM%3D&reserved=0>.
|
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. |
@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. |
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);
}
});
}
} |
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. |
Yes, all I'm actually proposing is that we get rid of the |
Adding In proposal #65 I believe I mentioned that I was trying to decide if we could eliminate the 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 ( 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 |
The big question is not whether we should combine I claim that this is ultimately a brittle pattern, no matter how you do it. Even if you My strawman and Tom's Java example illustrates that you don't need to do this. |
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 The other pattern I've seen referenced is the |
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 |
Building on @billba's and @tomlm's idea, It might be possible to abstract middleware responsibilities into a command based interface. This way, the 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 |
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... |
@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. |
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 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 |
I'm hoping the proposal does not include making me - a JavaScript developer - use |
@codefoster what would you have the JavaScript developer do? No matter what the |
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. |
I only mean that it shouldn't have to literally look like |
@codefoster was just about to clarify if that's what you're asking :) I had been thinking about adding |
@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. |
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. |
What you have here:
... 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. |
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 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. |
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? |
@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. |
Let me be more concrete about the dangers of the current approach, including the various mitigations you have proposed. Missing DependenciesGiven: bot
.use(Y)
.onReceive(myBotLogic) Let's say Removing DependenciesGiven: bot
.use(A)
.use(B)
...
.use(Y)
.use(Z)
.onReceive(myBotLogic) Is it safe to remove 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. |
@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. |
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 |
Yes, my repo is just a series of annotated samples that illustrate exactly these points. (Your examples swap my notions of First and foremost, I think your eye is trained to the simplicity of the current 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 If they prefer, the developer of a piece of middleware can also utilize a This To summarize, the current (I updated my proposal slightly to emphasize some of these points) |
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 let myThing;
bot(...)
.use(new MyMiddleware(myThing)
.onReceive(context => {
myThing.doMagic();
}) The only shortcomings I see are...
//app.ts
export myThing;
bot(...)
.use(new MyMiddleware(myThing)
.onReceive(context => {
myThing.doMagic();
})
//module1.ts
import { myThing } from './app'
myThing.doMagic() |
@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 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. |
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. |
After talking with @Stevenic in person we are a lot closer on this. We are now in agreement that At first glance this looks the same as what we were doing before - adding functionality to This removes the ugliest part of my proposal - a separate turn cache. P.S. @Stevenic good work pulling that rabbit out of your hat |
@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 |
THERE WERE HUGS INVOLVED AND I MISSED IT? oh man :( |
Both SDKs implemented this change in full, and so I am closing this. Thanks to everyone who participated. |
Currently middleware developers are encouraged to add functionality to
context
by adding properties and methods directly to it. This has a number of issues and, crucially, won't work in languages like Java.This repo details the problems and illustrates that there are other ways to accomplish the same thing: https://github.com/billba/middleware
The text was updated successfully, but these errors were encountered: