proposal: trimmed down context object

[Stevenic]

One of the biggest things we've talked about for M2 is stripping down the core botbuilder package to its essence. Thought I'd start with proposing a trimmed down context object:

see updated shape proposal #66 (comment)

export interface BotContext {
    /** The Bot object for this context. */
    bot: Bot;

    /** The received activity. */
    request: Activity;

    /** Queue of responses to send to the user. */
    responses: Partial<Activity>[];

    /** If true at least one response has been sent for the current turn of conversation. */
    readonly responded: boolean;

    /** Deletes an existing activity. */
    deleteActivity(id: string): Promise<void>;
    
    /** Sends a set of activities to the user. */
    postActivity(activities: Partial<Activity>[]): Promise<ConversationResourceResponse[]>;

    /** Flushes the response queue. Calls postActivity() */
    flushResponses(): Promise<ConversationResourceResponse[]>;

    /** Replaces an existing activity. */
    updateActivity(activity: Partial<Activity>): Promise<void>;
}

Let me go through and try to justify each part on the context object...

bot

This is really to provide access to the adapter as we've found you often need access to this. In particular, Middleware for MS Teams needs access to the underlying adapter to get an access token for calling other REST services they need to call.

request

Obviously provides access to the received activity.

responded

This is a flag that indicates whether a reply has been sent for the current turn. This is potentially useful from within middleware to know if you should run logic on the trailing edge of a request. For instance, QnA Maker can look at this flag on the trailing edge of the request and only call the service in a fallback case should no other logic in the bot respond.

responses and flushResponses()

This provides a means of queuing up outbound responses. Bots often end up sending more than one message to the user for the same turn and given that your bots logic might be spread across multiple components it's possible for several components to queue up an outgoing response on the same turn. Since the bots state is written out every time activities are sent it's best to queue up all of the responses so that you can collapse state updates down to a single write per turn.

The flushResponses() method gives you a way of manually flushing the response queue should you need to and it's also the method called when the context is about to be destroyed. All this method would do is call the new postActivity() method and then delete the flushed responses on completion.

postActivity(), updateActivity(), and deleteActivity()

These are new methods I'm proposing to explicitly perform CRUD operations against the underlying adapter in a way that goes through middleware. We recently realized that while we have a postActivity middleware pipe we're missing delete and update pipes. Currently updates and deletes for existing activities need to happen using the adapter directly which bypasses middleware meaning that updates & deletes won't be logged.

I have another proposal coming which will recommend we add new updateActivity and deleteActivity pipes to middleware. What's nice about these 3 new methods I'm proposing is that they will a) provide a direct means of posting activities (something we've been asked for) and b) they will align name wise with the names of the middleware pipes that will get run when each method is called. Should make it clearer what triggers the running of those pipes.

[Stevenic]

This would address issue #59

[cleemullins]

I'm a big fan of the overall direction towards simplicity. If I understand correctly, you're essentially proposing to turn BotContext into a CRUD + Flush container.

The only genuine issue I have is that we're getting awfully close to being formal "State Management". By that, I mean the the combination of adding responses, manipulating existing responses, then syncing that state with a remote user. We're walking up to that state management issue via CRUD operations, and not through a more deliberate approach.

In that regard, I really like the Action/Reducer pattern that Redux uses. I also wonder if we should treat sending more akin to replication, where the local state of the bot is replicated to the user wherever they might be.

Now, at the risk of undermining my own thinking, if we implement a strict CRUD layer here in the BotContext, I don't think anything prevents us from enriching it with another layer down the road.

At the Nitpicky level, I have a few minor concerns:

1. It's not clear, on the Send path, what's Sync and what's Async. That confusion is what leads to the need for FlushResponses. I believe a common pattern is:

• Send a Typing Indicator
• FlushResponses() to get it out to the user
• Assemble the actual response.
• Send that response.

That pattern should be semantically simple.

2. It's not clear that batching is possible beyond "Responses is an Array". The API doesn't offer any hints, and with the lack of clarity around sync/async it's a bit more confusing. Do I batch up "Typing" + "Delay" + "Actual Response" or do I flush them one at a time?

3. I'm not sure how much I like "responded". I 100% get the need for components to tell "Has anything happened", but some responses - such as "Typing" - would qualify even though they're not doing anything. I think we should consider some more semantically rich options.

4. I don't like PostActivity as a name. SendActivity seems much more obvious.

//Chris

[Stevenic]

So to be clear... The update and delete methods I've added are to better model the v3 protocol that the Bot Framework supports. If this looks like a more formal "State Management" that's because the v3 protocol supports this level of CRUD operation. You'll notice that I'm specifically NOT modeling read because that's not consistently supported across all channels.

With this proposed interface you could send a typing indicator directly using postActivity() which would be the same as adding it to the responses queue and calling flushResponses().

This layer is intended to be a very raw layer not unlike HTTP Request and Response. There's very little that's intuitive about those interfaces either. You basically need to read the documentation to understand the ins and outs of working at this raw of a layer. The idea is that you will add middleware plugins that extend the context object with members that are semantically more meaningful. There's nothing hear that even tells you what types of activities you can send. You need to add a piece of middleware that adds queueReply() and queueTyping() methods. The point though is that we want that opinion to be replaceable so it needs to layer on top of something more raw.

The addition of the responded property was to eliminate the need for a more explicit handled = true flag to indicate that the request has been handled and shouldn't be processed any further. Components like QnA Maker that have been added as middleware need the ability to know if they should run their logic or not. The con with the explicit approach of setting a flag to 'true' is that its super easy to forget to do this in your code. In most cases you can use the delivery of an activity as a good indicator that the request has been handled. Even the sending of a "typing" indicator is a good indicator that the request is being handled in some way. If we remove the automatic responded flag then we need to add back in an explicit handled = true which nobody, including myself, liked. This is a very real issue for components like QnA Maker so we can't just punt and say we're not going to solve their problem.

I'm not opposed to renaming postActivity() to sendActivity() but we should then also rename the middleware, bot, and adapter methods to Middleware.sendActivity(), Bot.send(), and Adapter.send(). They should all match for consistency purposes.

[Stevenic]

There's actually a further simplification that can be made here... I think we can remove the batching aspects of context.responses and context.flushResponses() and replace this with a piece of middleware:

export class BatchOperations implements Middleware {

    public contextCreated(context: BotContext, next: () => Promise<void>): Promise<void> {
        let responses: Partial<Activity>[] = [];
        context.batch = {
            reply: (text: string) => {
                responses.push({ type: 'message', text: text });
            },
            typing: () => {
                responses.push({ type: 'typing' });
            },
            flush: () => {
                const flushed = responses;
                responses = []; 
                return context.postActivity(flushed);
            }
        }

        return next()
            .then(() => context.batch.flush());
    }
}

This will actually simplify our internal middleware implementation because we no longer need to explicitly flush() the response queue as part of our pipeline processing and the lifetime of the context object becomes cleanly tied to the lifetime of the contextCreated pipeline. If you want batching semantics you can mimic the current behavior by simply using the middleware above as the first piece of middleware in your stack.

[Stevenic]

Updating this proposal to reflect the latest thinking spanning several other discussion threads:

export interface TurnContext {
    /** The Bot object for this context. */
    bot: Bot;

    /** The received activity. */
    request: Activity;

    /** If `true` at least one response has been sent for the current turn of conversation. */
    readonly responded: boolean;

    /** Gets a value previously cached on the context. */
    get<T = any>(key: string): T;

    /** Returns `true` if [set()](#set) has been called for a key. The cached value may be `undefined`. */
    has(key: string): boolean;

    /** Caches a value for the lifetime of the current turn. */
    set(key: string, value: any): this;

    /** Sends a set of activities to the user. */
    sendActivities(activities: Partial<Activity>[]): Promise<ResourceResponse[]>;

    /** Replaces an existing activity. */
    updateActivity(activity: Partial<Activity>): Promise<void>;

    /** Deletes an existing activity. */
    deleteActivity(id: string): Promise<void>;

    /** Registers a handler to be notified of and potentially intercept the sending of activities. */
    onSendActivities(handler: (activities: Partial<Activity>[], next: () => Promise<ResourceResponse[]>) => Promise<ResourceResponse[]>): this;

    /** Registers a handler to be notified of and potentially intercept an activity being updated. */
    onUpdateActivity(handler: (activity: Partial<Activity>, next: () => Promise<void>) => Promise<void>): this;

    /** Registers a handler to be notified of and potentially intercept an activity being deleted. */
    onDeleteActivity(handler: (id: string, next: () => Promise<void>) => Promise<void>): this;
}

Changes of Note

• Rename of BotContext to TurnContext.
• Added get(), has(), and set() methods that components can use to cache values for the lifetime of the current turn. These methods would follow the basic semantics of a Map. See issue fix middleware/context model #56
• Added sendActivities(), updateActivity(), and deleteActivity() methods to context object. The Bot Framework protocol supports not just sending activities but updating and deleting them as well. See issue proposal: new Middleware.updateActivity() and Middleware.deleteActivity() methods #67
• Removed the responses queue and related flushResponses() method. This added complexity to the perceived lifetime of the context object and can easily be added back on using middleware.
• Added onSendActivities(), onUpdateActivity(), and onDeleteActivity() methods for registering callbacks that want to be notified of and potentially intercept outbound calls from the bot. This will let us simplify middleware by getting down to a single pipe. See issue idea: consolidate 3 middleware pipes into 1 #74

[Stevenic]

One question about the proposed interface is that C# programmers tend to like really well spelled out methods like sendActivities() where from a JS perspective I'd lean towards something shorter like send(). How should we bridge differences like this?

[billba]

I don't know how to respond to this DCR. It seems like a catch-all summarizing a variety of proposals being discussed in other issues, although you are also discussing some of them here.

[Stevenic]

I know you don't like the on handlers. are their other specific line items you have issues with? I just started work on this last night so now is a great time to comment.

I'll be publishing an m2 branch sometime today and I would expect all of these various changes to just collect there until after the Milan hack is over. I implemented the above changes to the context object last night and so far they feel pretty good implementation wise. But nothing is set in stone of course.

[Stevenic]

BTW... The vast majority of DCR discussions is happening over in the .NET repo. I'm actually lagging behind them for once... lol

[billba]

I propose putting the cache functionality under a name context.cache, and making it an actual Map (this could be polyfilled for older versions of JS). In .net context.Cache would presumably be a Dictionary.

JavaScript

if (context.cache.has("mykey")) {
    value = context.cache.get("myKey");
    context.cache.delete("myKey");
}

C#

if (context.Cache.ContainsKey("myKey")) {
    value = context.Cache["myKey"};
    context.Cache.Remove("myKey");
}

To me that strikes the right balance between common naming across SDKs and language-specific idiom. It also gives context to the get/has/set functions, which by themselves don't telegraph much about their purpose.

[Stevenic]

This work is done.