DCR: Refactor Adapter

[billba]

Update: please go directly to the much revised version of this proposal below

Problem: Adapter currently conflates a number of concerns which makes it hard to understand as a concept, and creates unnecessary coupling. One metric for this is that different adapters are used quite differently, per #124.

For a console bot, the adapter enables middleware, sending/updating/deleting activities, and provides a message loop which runs your logic (and middleware) for you.

const adapter = new ConsoleAdapter(new MemoryStorage());

adapter
    .use(middlewareA)
    .use(middlewareB);

adapter.listen(context => {
    await context.sendActivity(`This durn thing works!`);
});

By comparison, a Bot Framework services bot also enables middleware and sending/updating/deleting activities, but instead of providing a message loop it has a function which runs your logic (and middleware) for you, one request at a time.

const adapter = new BotFrameworkAdapter(new MemoryStorage());

adapter
    .use(middlewareA)
    .use(middlewareB);

app.post('/api/messages', (req, res) => {
    adapter.processRequest(req, res, context => {
        await context.sendActivity(`This durn thing works!`);
    });
});

Proposal: Narrowly focus Adapter as a transport adapter, extracting out middleware, message loops, and logic execution.

First let's look at the new Adapter (I elided over arguments and return values for existing methods).

interface Adapter {
    sendActivity(...);
    updateActivity(...);
    deleteActivity(...);

    createContextForNewConversation(conversationReference): Promise<TurnContext>;
    createContextForExistingConversation(conversationReference): TurnContext;

    createContext(...): TurnContext;    // arguments are transport-specific
}

Note that Adapter is no longer responsible for running any bot logic.

As today, the first three methods are used by TurnContext:

class TurnContext {
    constructor(
        public adapter: Adapter,
        public activity: Activity
    ) {
        ...
    }

    ...

    onSendActivity(...);    // add to a pipeline that ultimately calls adapter.SendActivity
    onUpdateActivity(...);  // add to a pipeline that ultimately calls adapter.UpdateActivity
    onDeleteActivity(...);  // add to a pipeline that ultimately calls adapter.DeleteActivity

    ...
}

Then BYO message loop. This is important because developers already know how to make message loops and we don't need to reinvent or wrap these at this level. There's nothing stopping us (or anyone) from building an abstraction on top of this that does wrap a message loop, but it's important that our primitives do not.

const adapter = new BotFrameworkAdapter();

const myBotLogic = async (context: TurnContext) => {
    await context.sendActivity(`This durn thing works!`);
}

app.post('/api/messages', (req, res) => {
    myBotLogic(adapter.createContext(req, res));
}

Proactive messages are handled similarly:

const myProactiveLogic = async (context: TurnContext) => {
    await context.sendActivity(`Something happened!`);
}

const calledBySomeEventSomewhere = async (conversationReference: ConversationReference) => {
    await myProactiveLogic(adapter.createContextForExistingConversation(conversationReference));
}

Want middleware? You create your middleware pipeline independently of everything else. Nothing else in the system even knows middleware exists. It's a purely optional construct. In fact if you don't like the way we do middleware, you can roll your own.

class Pipeline {
    use(middleware: Middleware): Pipeline;
    run(context: TurnContext, handler?: (context: TurnContext) => Promise<void>): Promise<void>;
}

const pipeline = new Pipeline()
    .use(middlewareA)
    .use(middlewareB);

// request loop

app.post('/api/messages', (req, res) => {
    pipeline.run(adapter.createContext(req, res), myBotLogic);
}

// proactive messages

const calledBySomeEventSomewhere = async (conversationReference: ConversationReference) => {
    await pipeline.run(adapter.createContextForExistingConversation(conversationReference), myProactiveLogic);
}

Obviously this separating of concerns results in more code, but again that can be wrapped by any number of abstractions. The average bot developer may only ever see, e.g.:

const bot = new BotFrameworkBot();

bot.use(new BestMiddlewareEver());

bot.onRequest(async context => {
  await context.sendActivity(`This durn thing works!`);
});

const calledBySomeEventSomewhere = async (conversationReference: ConversationReference) => {
    await bot.createConversation(context => {
        await context.send(`Something happened!`);
    });
}

The better we separate concerns in our primitives, the more flexibility developers have to create high quality abstractions.

Comments please @drub0y @Stevenic @tomlm @cleemullins @garypretty @ryanvolum @cmayomsft @brandonh-msft

[billba]

This will greatly simplify and clarify the responsibility of a test adapter.

Now you can separately unit test:

• your middleware
• your bot logic
• proactive logic
• the adapter itself

And integration test:

• a middleware pipeline
• that pipeline plus your bot logic
• that pipeline plus your proactive logic

Now your test code owns its own message loop, which allows you to test your HamilBot as follows:

const adapter = new TestAdapter();

const script = [
    ["Excuse me, are you Aaron Burr, sir?", "That depends. Who's asking?"],
    ["What time is it?"], ["Showtime!"],
    ["Where's your family from?"], ["Not important there's a million things I haven't done but just you wait just you wait"]
]

describe('hamilBot', () => {
    it ('knows Hamilton really well', async (done) => {
        for await (const dialog in script) {
            await hamilBot(adapter.createContext({ type: 'message', text: dialog[0] }));
            expect(adapter.responses[0]).to.eql(dialog[1]);
        }
        done();
    });
});

Comment on test strategy please @GeekTrainer @jwendl @brandonh-msft

[GeekTrainer]

(This should probably be its own issue, as there's quite a bit to unpack here. I had started an issue with M1 for BotContext.)

I've been giving a fair bit of thought to this, and as we continue to trim down BotContext and Adapter and other classes to their barest components, we need to follow that strategy into testing. Because of the flexibility of v4, testing also needs to be that flexible. Someone might be using the primitives themselves, or Promptly, or some other implementation, each of which is going to require a different testing approach.

Towards that end, I agree with @billba at a high level. TestAdapter needs to "get out of the way", and let me do what I need to do. There are going to be times when i'll be at the root level of the bot, and times where I'll be testing directly into some helper function where I need to create a context on the fly.

Consider:

const bot = async (context: BotContext) => {
  // central routing here
  if (context.activity.text === 'create alarm') createAlarmTopic(context);
  if (context.activity.text === 'delete alarm') deleteAlarmTopic(context);
}

const createAlarmTopic = async (context: BotContext) => {
  // logic to create alarm
  // basic pseudo code here
  // there would be something better to handle alarm
  if(!alarm) {
    context.sendActivity(`What do you want to call your alarm?`);
    alarm = new Alarm();
  } else if(context.message.text) {
    alarm.title = context.message.text;
    context.sendActivity(`What time do you want to wake up?`);
  }
}

There's really two things that need to be tested

• Routing, which is done in bot
• Each topic (such as createAlarmTopic in my case) to ensure the correct messages are sent, the alarm is created, etc.

As a result, having an adapter where I simply send in messages (testAdapter.send('sampleText');) forces me into the root of the bot, which isn't really where I need to be to test createAlarmTopic.

I'd rather have something that looks like this:

describe('createAlarmTopic tests', () => {
  it('Creates alarm with title, and prompts for time', async () => {
    // assemble
    const adapter = new TestAdapter();
    const context = new BotContext(adapter);
    // alternatively: const context = adapter.createContext();
    context.activity = { type: 'message', text: 'Sample title' };
    const alarm = new Alarm();
    codeToSetAlarmInBot(alarm); // doesn't matter for what I'm trying to do

    // act
    await createAlarm(context);

    // assert

    // query adapter to see what was sent through context
    assert(adapter.sentActivities.length === 1);
    assert(adapter.sentActivities[0].text === 'What time do you want to wake up?');

    // check the alarm
    assert(alarm.title === 'Sample title');
    return Promise.resolve();
  });
});

The adapter is "getting out of the way", and just letting me check to see that the appropriate messages were sent. I'm then checking that the alarm was updated appropriately, which I'll need to do separately. This is going to be especially true as we disconnect state from context.

Regardless, TestAdapter needs to be consistent with the adapter. In other words, if it's going to be sendActivity and deleteActivity; it'd be a bit of a disconnect to be querying a responses property to see all of the activities that were sent.

[Stevenic]

Have you looked through my unit tests in the latest bits? If you look through the unit tests for botbuilder-core, botbuilder-core-extensions, and botbuilder you'll see that I pretty much NEVER use the TestAdapter for testing things. And when I do it's often just to new up a context object like you show...

Testing middleware with just a context object:

https://github.com/Microsoft/botbuilder-js/blob/master/libraries/botbuilder-core-extensions/tests/botState.test.js

Defining a new class called SimpleAdapter for testing that activities are delivered:

https://github.com/Microsoft/botbuilder-js/blob/master/libraries/botbuilder-core/tests/botContext.test.js

And for the BotFrameworkAdapter I specifically coded things so that I could override all of the network calls when testing:

https://github.com/Microsoft/botbuilder-js/blob/master/libraries/botbuilder/tests/botFrameworkAdapter.test.js

[Stevenic]

I'm striving for 100% code coverage across all of the core modules which has lead to me spending a lot of time thinking about how you test things and I agree. The TestAdapter is generally useful for integration level testing bot not so much for unit testing.

[billba]

Hmmm @GeekTrainer @stevenic my intention wasn't to initiate a broad conversation about testing (please start a separate issue for that!) but to ask whether this proposal for refactoring Adapter improved testability. To that end I wasn't really so much mooting a specific shape for TestAdapter, as observing that refactoring out the middleware pipeline, message loop, and bot logic execution seemed to give more fine-grained control.

To that end, @GeekTrainer I think you are saying you agree with this proposal, because you just use the TestAdapter to create a context which you run through your own logic, which was my point. In your code above you'd probably prefer to do:

const context = adapter.createContext({ type: 'message', text: 'Sample Title' });

because TestAdapter might need to add a few more fields to that activity to make things work. For that matter, you'd probably want TestAdapter to add some helpers for common cases:

const context = adapter.text('Sample Title');

And if your Topic depends on a specific middleware pipeline, you'd just change

await createAlarm(context);

to

await pipeline.run(context, createAlarm);

The fact is that both are viable options, and that's why it's important that the adapter doesn't run your bot logic for you -- as you demonstrate, you need to decide which part of your logic you want to run. Running the middleware pipeline and then running a specific piece of logic (rather than the root) is something that is not possible with the current system.

[billba]

After pondering this proposal for about 24 hours I still like it a lot, but it does have (at least) one consequence, which is that having a message loop allows Adapter to adapt the incoming message flow.

Specifically, #120 points out that there is an opportunity for ConsoleAdapter to insert conversationUpdate at the beginning of the activity stream, thereby allowing a more consistent approach to coding bots across adapters. I'm honestly not certain how critical a feature this is.

[GeekTrainer]

tl;dr - I mostly agree that your refactor of Adatper makes testing easier. I like being able to say adapter.createContext or some other helper as you propose. I'm not a fan of pipeline.run as I'd rather just have the freedom to call createAlarm.

FWIW, I did say right from the start that I thought testing should be its own issue (and even created one previously). I wasn't trying to hijack the thread, but provide a full answer to your question.

I do agree completely that the adapter should never run part of the logic, and shouldn't be the only entry point to the logic, both from a testability standpoint, but also a design perspective. Towards that end, I'm not necessarily a fan of pipeline.run(context, createAlarm) as I'm still having to call something else. I really just want to be able to call a specific method that takes a context for testing purposes, and everything else should really just get out of my way. The TestAdapter (or Adapter or whatever it winds up being called) should allow me to see what the context tried to send out the door, but little else.

I do like being able to say adapter.text('wibble') for those simple types of cases.

[billba]

The point is not that you have to call pipeline.run(...), the point is that you can either call your bot logic atomically, or run the middleware pipeline first. Freedom!

[GeekTrainer]

I got it! Then yes, I like the plan, assuming we make the change to createContext highlighted above.

[drub0y]

I'm just finally getting some time tonight to actually digest this. My first impression is that this definitely is heading in the right direction based on discussions we've had and would be a great next step for sure. What I like most is seeing the clear separation of the concerns. I don't know how to describe it just yet, but something about the adapter design still feels a little "upside down" for me. 🤔 It's late so I'm sure I'll be able to do a better job describing what it is after a full night's sleep.

Let me throw this out while I'm here though...

The way I had been thinking of it is that the thing that I was calling a transport would effectively just expose a stream of sorts... have you seen this new thing the kids are using these days called "observables"? So hot right now. 😝🤣 The stream would either be raw activities or maybe turn contexts or maybe even something specific for the transport layer along with a couple of methods for opening and closing the transport. Then how the underlying transport received activities, "normal" or proactive, would be completely hidden and it would just emit the items from the stream.

Now, if that thing's emitting items and we have a pipeline that represents the processing stack, all you need is something that glues them together by subscribing to the abstract transport stream and feeding each emitted item into the pipeline and that's what I was tentatively calling the "bot host". Here's a super rough ASCII diagram of what I'm going on about:

                                 +---------------------+            Messages
                                 |                     |        +---+ +---+ +---+
           +---------------------+    Transport        <--------+ M | | M | | M |
           |                     |                     |        +---+ +---+ +---+
           |                     +---------------------+
           |
+----------v-----------+
|                      |
|                      |
|                      |
|    Bot Host          |
|                      |
|                      |
|                      |
|                      |
|                      |
|                      |
|                      |
+----------+-----------+
           |
           |
           |
           |  Pipeline
+------+---+-----^------+
|      |         |      |
|  +---v---------+---+  |
|  |                 |  |
|  | Middleware A    |  |
|  +---+---------+---+  |
|      |         ^      |
|      |         |      |
|  +---v---------+---+  |
|  |                 |  |
|  |  Middleware B   |  |
|  +---+---------^---+  |
|      |         |      |
|      |         |      |
|  +---+---------+---+  |
|  |                 |  |
|  |     My Bot      |  |
|  +-----------------+  |
|                       |
+-----------------------+

[billba]

I bow before your ASCII art and will ponder your ideas.

[drub0y]

Small note: the diagram was totally focused on the incoming messages, but the bot host would similarly be configured with a transport for sending messages. Could be same transport as incoming, could be separate for completeness.

[billba]

Update: Please go to the newest revision of this proposal.

Update: changed TurnAdvisor to use an optional custom context.

Here's the latest proposal from @drub0y and me:

Problem: In M2 ActivityAdapter and Bot were combined into BotAdapter, conflating a number of concerns. This makes it hard to understand as a concept, more work to implement, and creates unnecessary coupling. It also means that TurnContext is now the low-level concept for the SDK, when previously it was Activity.

(A red flag for the current state of the adapter model is that different adapters are used quite differently, per #124.)

For a console bot, the adapter enables middleware, exposes sending/updating/deleting activities, and provides a message loop which runs your logic (and middleware) for you. It adapts incoming messages directly to TurnContext, which exposes a different version of the sending/updating/deleting activities exposed by the adapter itself. There is no way to get just activities straight out of the adapter, only TurnContexts (which do obviously contain activities, but the point is that our lowest level is now a much more opinionated construct).

const adapter = new ConsoleAdapter();

adapter
    .use(middlewareA)
    .use(middlewareB);

adapter.listen(context => {
    await context.sendActivity(`This durn thing works!`);
});

The Bot Framework adapter is similar, but instead of providing a message loop, has a function which runs your logic (and middleware) for you, one request at a time. Again, instead of Activities from the adapter you get TurnContexts .

const server = restify.createServer();
server.listen();

const adapter = new BotFrameworkAdapter();

adapter
    .use(middlewareA)
    .use(middlewareB);

server.post('/api/messages', (req, res) => {
    adapter.processRequest(req, res, context => {
        await context.sendActivity(`This durn thing works!`);
    });
});

Proposal: Revert to a variety of the M1 ActivityAdapter approach, which laser focuses on adapting transport differences into receiving/sending/updating/deleting activities, and creating new conversations.

interface ActivityAdapter {
    constructor(...); // arguments are transport-specific

    abstract sendActivity(activities: Partial<Activity>[]): Promise<ResourceResponse[]>;
    abstract updateActivity(activity: Partial<Activity>): Promise<void>;
    abstract deleteActivity(reference: Partial<ConversationReference>): Promise<void>;

    abstract createConversation(conversationReference: ConversationReference): Promise<ConversationReference>;

    abstract subscribe(handler: (activity: Activity) => Promise<void>);
}

You can build anything with just an adapter:

const adapter = new ConsoleAdapter();

adapter.subscribe(async activity => {
    if (activity.type === 'message')
        await adapter.sendActivity([{
            ... activity,
            type: 'message',
            text: `This durn thing works!`
        }]);
});

Different adapters construct differently, but are used exactly the same.

const server = restify.createServer();
server.listen();

const adapter = new BotFrameworkRestifyAdapter(server, '/api/messages');

adapter.subscribe(async activity => {
    if (activity.type === 'message')
        await adapter.sendActivity([{
            ... activity,
            type: 'message',
            text: `This durn thing works!`
        }]);
});

If you want to send a "proactive" message to someone other than the current user, you just change some fields of the outgoing activity:

adapter.subscribe(async activity => {
    if (activity.type === 'message' && activity.text === 'poll') {
        for await (const conversationReference in await getFriends(activity.user.id)) {
            await adapter.sendActivity([{
                ... applyConversationReference(activity, conversationReference),
                type: 'message',
                text: `Hey friend, what do you want on your pizza?`
            }]);
        }
    }
    ...
}

This is all a little too much work. We'd usually like to optimize for replying to a given user, and also introduce middleware pipelines for the *Activity methods. Enter TurnContext, which wraps both ActivityAdapter and a given Activity:

class TurnContext {
    constructor(
        public adapter: ActivityAdapter,
        public activity: Activity
    ) {
        ...
    }

    ...

    onSendActivity(...);    // add to a pipeline that ultimately calls adapter.SendActivity
    sendActivity(...);      // calls the above mentioned pipeline

    onUpdateActivity(...);  // add to a pipeline that ultimately calls adapter.UpdateActivity
    updateActivity(...);    // calls the above mentioned pipeline

    onDeleteActivity(...);  // add to a pipeline that ultimately calls adapter.DeleteActivity
    deleteActivity(...);    // calls the above mentioned pipeline

    ...
}

So now you can do:

adapter.subscribe(activity => {
    const context = new TurnContext(adapter, activity);
    context.sendActivity(`This durn thing works!`);
});

TurnAdapter simplifies this common pattern by wrapping ActivityAdapter's message loop:

const bot = new TurnAdapter(new ConsoleAdapter());

bot.subscribe(context => {
    context.sendActivity(`This durn thing works!`);
});

Frequently, a bot developer would like to extend TurnContext by adding properties and methods. TurnAdvisor can optionally use one that you provide:

const bot = new TurnAdapter(
    (adapter, activity) => new MyContext(adapter, activity, conversationState),
    new ConsoleAdapter()
);

const conversationState = new ConversationState<{ count: number }>(new MemoryStorage());

// now context is type MyContext
bot.subscribe(context => {
    context.conversationState.count++;
    context.sendActivity(`This durn thing works!`);
});

A web service bot might support more than one adapter. TurnAdapter can merge them into a single stream. This is very useful.

const bot = new TurnAdapter(
    new SomeServiceAdapter(...),
    new AnotherServiceAdapter(...),
);

bot.subscribe(context => {
    await context.sendActivity(`This durn thing works!`);
});

Now proactive messages are a little simpler too:

bot.subscribe(async context => {
    if (context.request.type === 'message' && context.request.text === 'poll') {
        for await (const conversationReference in await getFriends(context.request.user.id)) {
            new TurnContext(context.adapter, conversationReference)
                .sendActivity(`Hey friend, what do you want on your pizza?`);
        }
    }
    ...
}

Want middleware? You create your middleware pipeline independently of everything else. Nothing else in the system even knows middleware exists. It's a purely optional construct. In fact if you don't like the way this does middleware, you can roll your own.

class Pipeline {
    use(middleware: Middleware | MiddlewareHandler): Pipeline;
    run(handler?: (context: TurnContext) => Promise<void>): (context: TurnContext) => Promise<void>;
}

const pipeline = new Pipeline()
    .use(middlewareA)
    .use(middlewareB);

bot.subscribe(pipeline.run(async context => {
    await context.sendActivity(`This durn thing works!`);
}));

You may want to run this pipeline before other code, e.g. in the "proactive" part of the pizza bot:

bot.subscribe(pipeline.run(async context => {
    if (context.request.type === 'message' && context.request.text === 'poll') {
        for await (const conversationReference in await getFriends(context.request.user.id)) {
            await pipeline.run(context => {
                await context.sendActivity(`Hey friend, what do you want on your pizza?`);
            })(new TurnContext(context.adapter, conversationReference));
        }
    }
    ...
}));

The nice thing about extracting out the middleware pipline from ActivityAdapter and TurnAdapter is that you can have multiple pipelines for different purposes (e.g. one for requests, one for proactive messages). This is also a big win for unit testing.

Obviously this separating of concerns results in more code, but again that can be wrapped by any number of abstractions. The average bot developer may only ever see, e.g.:

const bot = new ConsoleBot()
    .use(middlewareA)
    .use(middlewareB)
    .onRequest(async context => {
        if (context.request.type === 'message' && context.request.text === 'poll') {
            for await (const conversationReference in await getFriends(context.request.user.id)) {
                bot.continueConversation(conversationReference, context => {
                    await context.sendActivity(`Hey friend, what do you want on your pizza?`);
                });
            }
        }
    });

The better we separate concerns in our primitives, the more flexibility developers have to create high quality abstractions.

Comments please @drub0y @Stevenic @tomlm @cleemullins @garypretty @ryanvolum @cmayomsft @brandonh-msft

[garypretty]

Thanks @billba and @drub0y for the detailed and clear proposal. On the face of it what you are suggesting makes sense to me and I really like the idea of being able to have different middleware pipelines for different purposes (pro-active, request etc.). Importantly you highlight that the average developer may only ever see pretty much what they would see in the current implementation, but with the added flexibility under hood if they need it. I like it.

[Stevenic]

Taking from an internal email over this...

one of the key things I don't see addressed here are the reasons we moved away from the M1 approach in the first place. That's because we kept running into the need for the Adapter to extend the context object and we weren't finding great ways of doing that. In particular, MS Teams has a need to extend the context object from their adapter and I recently built two prototype adapters for Alexa and Google Home that had similar needs (I'll go into specifics below.) In the M1 architecture where we had the adapter & bot split the adapter was not allowed to know about either the Bot or the Context object which made it difficult for us to solve the issues we were running into. With M2 we combined these two together which meant that the adapter was now factoring the context object making it easier to extend things.

So more specifics on the types of Adapter extensions we’ve seen the need for. In the case of Teams they have a suite of Middleware they’re building for doing things like processing @mentions and working with tenants and channels. A lot of the middleware they want to build needs to call their backend service but it needs per/tenant credentials from the bot to make that work. We spent an hour or two with their devs talking through various approaches and every angle we came at it using the M1 architecture had a seam of some sort. Either it worked great in the reactive case but felt clunky in the proactive case or vice versa.

The core need for Teams is the ability to cache credential information for the lifetime of a turn. Since the context lives for a turn, caching it off the context feels like the most natural fit. The issue with M1 is that the adapter had no knowledge of either the bot or the context so any approach we took felt un-natural. With M2 the adapter is responsible for factoring the context so solving this issue is trivial. They can cache anything they want off the context and can even re-type it to be a MSTeamsContext object if they want.

I ran into a similar issues with my Alexa & Google Actions adapter. Both Alexa & Google have the ability to persist state for the skill that’s remembered per turn. While it’s true that we have our own state storage abstractions, as a developer I should be free to use the one that Alexa & Google provide if I so chose. I somehow needed to round-trip that storage through the bot developers logic and it’s super important that this be bound to the lifetime of the turn. So for both adapters I just extended the context with the built-in state objects and then re-typed them to be AlexaContext and GoogleActionContext instances.

One possible approach would be to return to the M1 approach of separating the Adapter and Routing logic but to keep the change to have the adapter factory the context object. This would retain the ability for the adapter to extend the context object in anyway it sees fit but then create a standardized interface for plugging the adapter into other stuff like a middleware pipe. It would also mean that the context object continues to be the god object that higher level components can be designed to take as input. The adapter would just need to make sure that it can automatically factory a context object in the reactive case and the developer can easily factory a context object in the proactive case.

I actually like the subscribe() method you propose as a better name but the reason I originally rejected that name in M1 is that it implies that you can have multiple subscribers and I don’t see how you can support that without adding a ton of complexity. The “invoke” case alone almost mandates that you should only ever have a single subscriber.

[billba]

we kept running into the need for the Adapter to extend the context object

They ... can even re-type it to be a MSTeamsContext object if they want.

First of all, there is no notion of a special adapter for Teams, right? There is just BotFrameworkAdapter, which adapts the Bot Framework connector service, which interfaces with a number of channels, one of which is Teams. If we take this to its logical conclusion this single adapter could spit out a different type of context for each channel. But the whole point of the Bot Framework is write-once, run-on-many-channels, so everything goes through a common message loop, so you have to coerce each of these differently typed context objects to TurnContext. And so I guess any time you want to use channel-specific features you'd coerce it back to the channel-specific typing? What if a bot developer wants to use her own definition of context? Which *Context class would she inherit from?

Yikes. I believe we should consider this an anti-pattern in the biggest way.

I will use my proposal to illustrate what I consider to be the correct way to solve this problem, while maintaining strong typing and a clean separation of concerns.

(The following sounds like we're picking on Teams, but I'm really just using them as a (friendly) proxy for anyone who wants to do stuff like this.)

Teams can publish a helper service, shaped very much like the BotState service. As with BotState, they could cache items in the context, but using an opaque key that discourages/prevents bot developers from accessing them directly.

I'm making the details up, but in broad strokes:

// MSTeams

export interface CoolTeamsData {
    ...
}

export class BotHelper {
    constructor (public someCoolTeamsData: CoolTeamsData) {
    }

    aCoolTeamsMethod(...) {
        ...
    }
}

export class BotHelperService implements Middleware {
    constructor (...) {
    }

    key = Symbol();

    async read (context: TurnContext) {
        let coolTeamsData: CoolTeamsData;

        if (context.has(this.key)) {
            coolTeamsData = context.get<CoolTeamsData>(this.key);
        } else {
            coolTeamsData = // presumably call a Teams REST endpoint
            context.set(this.key, coolTeamsData);
        }

        return new BotHelper(coolTeamsData);
    }

    get (context: TurnContext) {
        return context.has(this.key)
            ? new BotHelper(context.get<CoolTeamsData>(this.key))
            : undefined;
    }
}

// my bot

import * as Teams from { MSTeams }
const teamsService = new Teams.BotHelperService(...);

const bot = new TurnAdapter(new BotFrameworkAdapter(...));

bot.subscribe(pipeline.run(async context => {
    const teams = await teamsService.read(context);
    // if our bot only uses the Teams channel we can skip this check
    if (teams) {
        teams.aCoolTeamsMethod();
        var coolData = teams.someCoolTeamsData;
    }
    someFunction(context, teams); 
}));

This works, but it's inconvenient to have to pass both context and teams to what might be a deep tree of child functions. Each those functions could call const teams = teamsService.get(context); but that's differently inconvenient.

In M1 we solved this by injecting new data and methods into context directly. In M2 the bot developer instend extends the TurnContext class, adding the pieces they want:

// MSTeams

export class MSTeamsContext extends TurnContext {
    teams: Teams.BotHelper;

    private constructor(
        adapter: ActivityAdapter,
        activity: Activity,
    ) {
        super(adapter, activity);
    }

    async from (
        adapter: ActivityAdapter,
        activity: Activity,
        teamsService: Teams.BotHelperService,
    ) {
        const mycontext = new MSTeamsContext(adapter, activity);
        mycontext.teams = await teamsService.read(this);
        return mycontext;
    }
}

// my bot

const bot = new TurnAdapter(
    (adapter, activity) => MSTeamsContext.from(adapter, activity, teamsService),
    new BotFrameworkAdapter(...)
);

bot.subscribe(context => {
    // if our bot only uses the Teams channel we can skip this check
    if (context.teams) {
        context.teams.aCoolTeamsMethod();
        var coolData = context.teams.someCoolTeamsData;
    }
    someFunction(context);
});

This solves the problem, because now the deep tree of child functions can all take a context argument of type MSTeamsContext.

Teams could and should publish such a MSTeamsContext, but they also need to publish the underlying BotHelperService because a bot developer may want to create their own MyContext which uses a variety of services.

[billba]

Just to combine several points made above, an interesting and convenient thing about my proposal is that you can combine BotFrameworkAdapter with other ActivityAdapters to get a new level of write-once, run-on-many-channels:

const bot = new TurnAdapter(
    (adapter, activity) => new MyContext(adapter, activity, teamsService, skypeService, alexaService),
    new BotFrameworkAdapter(...),
    new AlexaAdapter(...)
);

bot.subscribe(pipeline.run(context => {
    // do common stuff
    await context.sendActivity(`This works for all channels!`);

    // do channel-specific stuff
    switch (context.request.channelId) {
        case 'msteams':
            // use context.teams
            break;
        case 'skype':
            // use context.skype
            break;
        case 'directLine':
            // doesn't have any specific channel features
            break;
        case 'alexa':
            // use context.alexa
            break;
    }
}));

[garypretty]

I have not been keeping track of the specific updates on this thread, but I saw that there were some specific references to requirements from the Teams channel etc. This may be of no relevance, but there is a PR open in the dot net repo that might be relevant - https://github.com/Microsoft/botbuilder-dotnet/pull/356

[billba]

@garypretty thanks that does seem very relevant. @RamjotSingh I think you should read through this thread and chime in. I believe that there is the opportunity to cleanly separate adapters, activities, context, and channels. It may even be that BotFrameworkAdapter could have literally zero Teams-specific code in it, which seems like a good idea.

[billba]

It has bothered me that custom context seem like a bolt-on, so I rejiggered TurnAdapter in the samples above to take a factory function. It defaults to

(adapter, activity) => new TurnContext(adapter, activity)

but you can provide your own. I like that now custom context is a first-class concept, and the presence of that argument is a strong hint about one important way you can customize your bot development experience.

I updated the samples above to reflect this change.

[RamjotSingh]

Since my knowledge of Js is limited at the moment. I am going to be talking about the problem and solution from C# side. Hoping we can apply the same to JS as well :).

@billba The changes I made to BotFrameworkAdapter are not teams specific. All I did was provide a way for constructs (Middleware or Callbacks) down the pipeline be able to pull Auth details so that they can build their own connectors etc. Second change I made was to push the ConnectorClient in the pipeline so that if someone wants to access the ConnectorClient for operations that the adapter does not expose. As far as teams is concerned we have to build our own client(TeamsConnectorClient) anyway because Teams supports constructs which are not part of the core BotFramework. The plan I have for now for Teams extenstion SDK does not involve overriding BotFrameworkAdapter or the context (infact I ended up removing the BotFrameworkTurnContext that I had at one point introduced yaay). That being said I think the main problem we are dealing with here is the customizability of things and how inherently there I no unifying system that would allow to easily port stuff around the adapter or essentially write in a manner that you could give it any adapter and things will just work.

At a higher level the problem statement I would put today is that I should ideally be able to write code in a way that in my controller I ask for an adapter (BotAdapter) and regardless of the adapter be it Test or BotFramework adapter my code just works. This is obviously not possible today because of how the functions are different in BotFrameworkAdapter as compared to BotAdapter. These changes in turn are a result of the fact that BotFramework has 2 extra constructs which are basic to BotFrameworkAdapter Auth (Header + BotAppId), HTTPPipeline (ServiceUrl).

Referring to the updated model I agree with the fact that not having access to outgoing Activity is weird and it limits the capability of what a developer can do with it. For example what if you wanted to set ChannelData on the activity or mention people etc. This is again a result of the ConnectorClient model we have which only applies to BotFrameworkAdapter.

Even with all of these issues I do agree with @Stevenic that the newer M2 model has made adapters aware and not just empty shells allowing things to be run in a more context aware mode.

I like the @billba 's approach of being able to access channel specific functionality in your code and handle it accordingly but the whole having multiple adapters seems weird to me. It doesn't solve the basic problem we have today which is unification of things. IMO we should have one adapter and Middlewares should take of the channel specific stuff.

Yesterday I was trying to think of a way I could unify BotFrameworkAdapter under a common umbrella. Following are the functions which are different in BotFrameworkAdapter

• ProcessActivity - Takes additional Auth header
• CreateConversation - Takes ServiceUrl and BotAppId additional (it asks for credentials but we can figure it out from the Credential Provider so effectively only BotId)
• ContinueConversation - BotAppId

For ContinueConversation the solution is relatively simple since BotFrameworkAdapter serializes the info it can serialize additional info and then deserialize it and everything just works. For rest 2 what does the issue actually come from is the fact that BotFrameworkAdapter is not just an adapter. It is also dealing with stuff it ideally should not be i.e. Auth. Auth is a BotFramework concept yes but Adapter should not be dealing with it. It is not necessary for Adapter to work, all it needs is a way to create ConnectorClient which require 3 things; BotAppId, BotAppPassword and ServiceUrl. These in turn are not dependent on Auth either (we can technically get a unauthenticated request and send an authenticated request back, no one should be stopping us from doing that). Hence my proposal? Keep the adapter model we have but Adapter's works needs to be limited to that it can do. Adapters should do only 2 things

• Run the middleware + call back pipeline
• Provide a measure to talk back to your caller (be it HTTP, Console, Test etc.) By the measure above BotFrameworkAdapter needs to stop dealing with Auth and have assumption that Auth is going to be passed to it. In C# we can do this by pushing things down the request context. In JS I am unsure if something similar exists. This also sets up a pattern of what an Adapter is supposed to do. If someone wants to write their own custom adapter (for having a Bot in WindowsService maybe?) they know what things they need to take care of.

[billba]

@RamjotSingh I appreciate your feedback.

[navzam]

I like the return to Activity as the low-level concept as opposed to any kind of context. Activity has everything from the incoming message and nothing more, so it's a natural primitive. I started playing with M2 a couple days ago, and the first thing that struck me was the absence of an activity handler in BotAdapter. And the resulting mismatch between ConsoleAdapter and BotFrameworkAdapter. So I like the common subscribe (though I'd prefer a different name, maybe something prefixed with on).

I'm struggling to draw the line between the base ActivityAdapter and implemented adapters. For example, are update/delete common enough across "transports" to justify being part of the base adapter? I think of "adapter" as something that supports creating conversations, sending activities, and handling received activities. I can picture changing my adapter from BotFrameworkAdapter to another and getting runtime errors simply because most adapters out there don't implement update/delete.

[GeekTrainer]

One thing I'd like to add to the discussion is if we are going to go with subscribe as the way to capture incoming activities, I'd like a more rxJS style with filter. In particular, I find it clunky to have the following code:

adapter.subscribe(context => {
  if(context.activity.type === 'message') {
    // do stuff
  }
});

It seems silly to be filtering after the "event has fired" so to speak. I'd rather have a more fluent:

adapter
   .filter(ctx => ctx.activity.type === 'message')
   .subscribe(context => {
      // do stuff
   });

[Stevenic]

@GeekTrainer your comment illustrates my issue with the word subscribe() it implies that you can have multiple listeners to the output of an adapter but given the semantics of the Bot Framework protocol you really can't have multiple listeners.

The issue is that the BF protocol is really two protocols that work entirely differently. There's the main flow that you're used to where messages and events come in, they're distributed to the bots logic, then the bot may or may not send replies to the user at their leisure using a separate POST back to the channels server. In this flow we could, theoretically, immediately ACK the received activity with a 202 upon receipt and then distribute a copy of the activity to everyone that has subscribed. I say theoretically because we don't immediately ACK the request and instead hold it open until we see that the bot has fully processed the request and the turn is complete. The reasons for this will hopefully be clear in a moment.

The second flow we have to support is the "invoke" flow. When the adapter receives an "invoke" activity the protocol changes such that we need to return the bots reply as the body of the HTTP response to the revoke. So instead of immediately sending a 202 we have to hold open the request, dispatch the "invoke" to the bots logic, listen for it to respond (which we save off to a map), and then upon the turn completing we have to return the bots response as the body of a 200 message.

While there's a very good chance you've never had a bot that's needed to handle "invoke" it's part of our protocol and we have a number of features that require it. Payments being one and MS Teams are heavy users of invoke for some of their more advanced features. There's also a slack feature called "ephemeral messaging" which requires the use of invoke. Furthermore, to support channels like Alexa & Google Home we'll need an activity type similar to "invoke" as both those channels require the bots response be delivered inline.

Invoke is just one of the reasons that we hold the request open until the turn completes the other is Azure Functions. When we run in Azure Functions the user is billed from millisecond the request is received until the millisecond the response is returned so to keep the billing accurate we're required by Azure Functions to complete all processing prior to returning a response. And beyond billing the moment we return a response in Azure Functions we become a candidate for recycle so we have to hold open the request to prevent your bots code from being abruptly halted during the execution of its logic.

So what does this have to do with supporting multiple subscribers? Technically we could support multiple subscribers so long as every subscriber returned a Promise which we then awaited so that the adapter knows when every subscriber has completed and it's safe to return the bots response. But then the question is do you run the subscribers in series or in parallel? This is just a lot of complexity for EVERY adapter to have to deal with so our M1 protocol kept it simple and said there's a single listener (the Bot) to the adapter and that listener returns a Promise that the adapter can wait on to know that all processing has completed and it's safe to return a response for the request.

[billba]

@navzam update and delete are common enough that they're worth having in the base ActivityAdapter, and yes, they would probably throw when unsupported. I'm not sure what the plan is for BotFrameworkAdapter which aggregates channels which do and channels that don't support those operations.

[billba]

I'm not attached to subscribe. That name is by far the least important part of this proposal. If that's the only thing blocking acceptance of this proposal, I'd be happy to change it to almost anything else.

@Stevenic I don't think using it means we have to support multiple subscribers in this release. In the future we can think this through and consider doing so.

@GeekTrainer an obvious thing to do would be to implement ActivityAdapter/TurnAdapter using Observable, but that is explicitly not part of this proposal. Again, something that could be added in a future release. That said, I think it would be unlikely in the extreme to filter activities into your main dispatcher as you illustrate. Your bot is, frankly, going to need to deal with all of them, one way or another. So I think you just need to get used to switching on the activity type.

[Stevenic]

I can comment on the Teams part of this separately but wanted to start with commenting in detail on teh original proposal. I really need to be able to do [cil] but since I can't I'll try to pick apart this proposal piece by piece and enumerate my issues and even propose a couple of changes I'd personally be ok with:

AdapterConstruction

The first issue I have is the proposed way you construct an adapter:

const adapter = new BotFrameworkRestifyAdapter(server, '/api/messages');

As clean as this code looks we actually started with this patern in v1 of the SDK and the feedback was clear (on the Node side at least) that developers did not like us hiding the express/restify routing chain from them and for good reasons.

• We have several customers that have needed to inject a hook before adapter.listen() so that they can do their own logging of the recieved activity or other pre-processing like tunneling through of tokens found in request headers (they'll copy a header into the activities channelData for instance.)
• This means the adapter can only work in a traditional Express/Restify style web server. We have several customers that host their bots on AWS Lambda and the way they make that work is to call the closure returned by adapter.listen() with a polyfill that makes Lambdas context object look like an Express/Restify style Request & Response object. The same thing happens in our own AzureFunctionsAdapter. I simply derive from the base ChatAdapter class and override listen() with a polyfil. That would be more complicated now due to the assumption that there's a full blow server object passed into the constructor.

TurnContext

The proposed signature for TurnContext is exactly the signature we have today so that seems like a no-op. It's all about who factories it.

const context = new TurnContext(adapter, activity);

TurnAdapter

This is honestly where things start to leave a bad taste in my mouth.

const bot = new TurnAdapter(new ConsoleAdapter());

First I look at that line of code in issolation and I'm like "What's the difference between a TurnAdapter and the ConsoleAdapter? Why isn't the ConsoleAdapter just doing what TurnAdapter does so I don't need to know two concepts?" Then once I understand the role of TurnAdapter my concern is that while I can see some convenience that it provides for the Bot Developer it only provides it for a Bot Developer. A component developer gets none of those benefits and it start making our SDK feel asymetical. In fact as a Bot Developer I can't even take a bunch of code that I wrote for my bot and re-package it as a component because it was all written with the asusmption that I had this TurnAdapter spitting out a custom context object for me.

And the aggregation of multiple adapters in TurnAdapter would work ok in the reactive case but not in the proactive case, at least not as you have it coded. The issue is you need a complete list of all the possible channelId's supported by a given adapter otherwise you dont know which adapter should be used to route a given proactive message. And the tricky thing with that is for the BotFrameworkAdapter it's technically an open ended list. At a minimum you would need the dev to explicitly provide this mapping.

const bot = new TurnAdapter(
    new SomeServiceAdapter(...),
    new AnotherServiceAdapter(...),
);

Pipeline Separation

I'm still mulling this separation over and I could almost be convinced of it. Largely because it already exists a free standing MiddlewareSet class that you can run(). The on the fence part for me is that I look at your code and I can't imagine a real production bot that wouldn't have at least some middleware so I'm like why do I explicitly have to run the middleware pipeline myself? When would I not want to do that?

bot.subscribe(pipeline.run(async context => {
    await context.sendActivity(`This durn thing works!`);
}));

Your multi-pipeline argument is probably the better argument for me. So I think I could preatty easily be talked into removing the MiddlewareSet from the adapter and then changing your normal reactive code to something like:

import { BotFrameworkAdapter, MemoryStorage, ConversationState, MiddlewareSet } from 'botbuilder';
import * as restify from 'restify';

// Create server
let server = restify.createServer();
server.listen(process.env.port || process.env.PORT || 3978, function () {
    console.log(`${server.name} listening to ${server.url}`);
});

// Create adapter
const adapter = new BotFrameworkAdapter( { 
    appId: process.env.MICROSOFT_APP_ID, 
    appPassword: process.env.MICROSOFT_APP_PASSWORD 
});

// Define conversation state shape
interface EchoState {
    count: number;
}

// Add conversation state middleware
const conversationState = new ConversationState<EchoState>(new MemoryStorage());
const middleware = new MiddlewareSet(conversationState);

// Listen for incoming requests 
server.post('/api/messages', (req, res) => {
    // Route received request to adapter for processing
    adapter.processRequest(req, res, middleware.run(async (context) => {
        if (context.request.type === 'message') {
            const state = conversationState.get(context);
            const count = state.count === undefined ? state.count = 0 : ++state.count;
            await context.sendActivity(`${count}: You said "${context.request.text}"`);
        } else {
            await context.sendActivity(`[${context.request.type} event detected]`);
        }
    }));
});

That would leave the adapter responsible for factoring the context object and now middleware is an optional thing. I'll go a step further even and get this back closer to M1 by saying we could collapse the listen part down to this:

const adapter = new BotFrameworkAdapter(keys);
server.post('/api/messages', adapter.listen(middleware.run(async (context) => {
    if (context.request.type === 'message') {
        const state = conversationState.get(context);
        const count = state.count === undefined ? state.count = 0 : ++state.count;
        await context.sendActivity(`${count}: You said "${context.request.text}"`);
    } else {
        await context.sendActivity(`[${context.request.type} event detected]`);
    }
}));

And for the ConsoleAdapter it would be:

const adapter = new ConsoleAdapter();
adapter.listen(middleware.run(async (context) => {
    if (context.request.type === 'message') {
        const state = conversationState.get(context);
        const count = state.count === undefined ? state.count = 0 : ++state.count;
        await context.sendActivity(`${count}: You said "${context.request.text}"`);
    } else {
        await context.sendActivity(`[${context.request.type} event detected]`);
    }
});

[billba]

@Stevenic Great. Let's start by focusing on your concerns about asymmetry, because they actually transcend this proposal.

The whole point of the M2 'skinny context' architecture is that every piece of code in the system -- middleware, bot logic, or library -- has access to a TurnContext and a variety of services (e.g. ConversationState, TeamsHelperService, AlexaService), and that's all they need. Period. So there is no asymmetry whatsoever. None.

This is a totally common pattern:

// context: TurnContext
const state = conversationState.get(context);
const teams = teamsService.get(context);

await teams.doStuff(state.foo);

This identical code can be used, without alteration, in middleware, bot logic, and libraries. No asymmetry.

(Recall that the reason we did this was specifically to put middleware and bot logic on the same footing. No one would have to guess as to what was in the context object. It would always be the same thing.)

The most important point here is that no one -- middleware, bot logic, libraries -- ever needs anything other than TurnContext and a set of services.

Are we agreed on this? I ask because you specifically said "we kept running into the need for the Adapter to extend the context object". I am directly asking now: is there any problem that cannot be solved by TurnContext and a transport-specific service? I'm not talking about solving for convenience or intuitiveness, I'm asking is the combination of TurnContext and a transport-specific service sufficient to solve any problem?

[Stevenic]

@billba I think we're completely aligned that the skinny TurnContext is a god object for a tune that everyone (bot developers and component builders included) can depend on.

My comment about asymmetry, and maybe there's a better word I could use, is that what I'm not crazy about is that SHOULD a bot developer chose to extend the context object and then later they decide they would like to re-package their code as component for others to re-use they're going to have to figure out how they package up their context extensions as well. Your TurnAdapter class looked like it was going to make that difficult because what I was reading was that the context extensions would occur in the TurnAdapter which they're obviously not going to be able to influence from within their component package.

Where I'm coming at from this is that with the botbuilder-dialogs library I have a CompositeControl class that makes it trivial to re-package any subset of a bots logic as a control that can either use internally within your bot or export as a package. I tend to use that a litmus test to know when I have things properly factored or at least close to properly factored.

[Stevenic]

Back on the topic at hand... I explained my proposed set of changes to @tomlm to break out the MiddlewareSet from the adapter and then move back to a listen() method and he's at least fine with breaking out the middleware pieces. Should we call that piece a done deal and I just make those changes? I'm working on all that code right now making the other tweaks we discussed so I'd like to just fold that into my current change set.

[billba]

Here's my latest latest proposal, which I believe takes into account all the concerns expressed by @Stevenic. Not only does this proposal yield a cleaner separation of concerns, but it buys a new feature: simple support for combining multiple adapters.

Problem

In M2 ActivityAdapter and Bot were combined into BotAdapter, conflating a number of concerns. This makes it hard to understand as a concept, more work to implement, and creates unnecessary coupling. It also means that TurnContext is now the low-level concept for the SDK, when previously it was Activity.

(A red flag for the current state of the adapter model is that different adapters are used quite differently, per #124.)

For a console bot, the adapter enables middleware, exposes sending/updating/deleting activities, and provides a message loop which runs your logic (and middleware) for you. It adapts incoming messages directly to TurnContext, which exposes a different version of the sending/updating/deleting activities exposed by the adapter itself. There is no way to get just activities straight out of the adapter, only TurnContexts (which do obviously contain activities, but the point is that our lowest level is now a much more opinionated construct).

const adapter = new ConsoleAdapter();

adapter
    .use(middlewareA)
    .use(middlewareB);

adapter.listen(context => {
    await context.sendActivity(`This durn thing works!`);
});

The Bot Framework adapter is similar, but instead of providing a message loop, has a function which runs your logic (and middleware) for you, one request at a time. Again, instead of Activities from the adapter you get TurnContexts .

const server = restify.createServer();
server.listen();

const adapter = new BotFrameworkAdapter();

adapter
    .use(middlewareA)
    .use(middlewareB);

server.post('/api/messages', (req, res) => {
    adapter.processRequest(req, res, context => {
        await context.sendActivity(`This durn thing works!`);
    });
});

Proposal

Revert to a variety of the M1 ActivityAdapter approach, which laser focuses on adapting transport differences on a specific set of functionality via the Activity object.

class ActivityAdapter {
    constructor(...); // arguments are transport-specific

    abstract sendActivity(activities: Partial<Activity>[]): Promise<ResourceResponse[]>;
    abstract updateActivity(activity: Partial<Activity>): Promise<void>;
    abstract deleteActivity(reference: Partial<ConversationReference>): Promise<void>;

    abstract createTurn(..., handler: (activity: Activity) => Promise<void>): Promise<void>; // arguments are transport-specific

    abstract onTurn(handler: (activity: Activity) => Promise<void>) => Promise<void>;

    abstract createConversation(conversationReference: ConversationReference): Promise<ConversationReference | undefined>;    
    abstract createTurnForConversation(conversationReference: ConversationReference, handler: (activity: Activity) => Promise<void>): Promise<boolean>;
}

onTurn

The beating heart of ActivityAdapter is the onTurn() message loop:

const consoleAdapter = new ConsoleAdapter();

consoleAdapter.onTurn(async activity => {
    await consoleAdapter.sendActivity([{
            ... activity,
            type: 'message',
            text: `This durn thing works!`
        }]);
});

This calls adapter.createTurn(...) on every request.

customizing onTurn()

A message loop is a personal thing, and most ActivityAdapters will allow you to replace the default with your own. For some, like BotFrameworkAdapter, there may not even be a default:

const server = restify.createServer();
server.listen();

const botframeworkAdapter = new BotFrameworkAdapter(handler => server.post('/api/messages', (req, res) => botframeworkAdapter.createTurn(req, res, handler));

botframeworkAdapter.onTurn(async activity => {
    await botframeworkAdapter.sendActivity([{
            ... activity,
            type: 'message',
            text: `This durn thing works!`
        }]);
});

Open Issue: This sort of thing is normal for functional programmers, but may feel uncomfortable for others.

Open Issue: Obviously this whole thing could more elegantly be done with Observables.

Proactive messages

If you want to send a "proactive" message to someone other than the user who send an activity, use the createTurnForConversation method:

adapter.onTurn(async activity => {
    if (activity.type === 'message' && isRudeComment(activity.text)) {
        await adapter.createTurnForConversation(getManagerConversationReference(activity.user.id), async activity => {
            await adapter.sendActivity([{
                ... activity,
                type: 'message',
                text: `Please be aware that your employee is using some salty language.`
            }]);
        });
    }
})

This method returns a Promise of false if the channel for the supplied conversation reference doesn't match the adapter.

TurnContext

This is all a little too much work. We'd usually like to optimize for replying to a given user, and also introduce middleware pipelines for the *Activity methods. Enter TurnContext (unchanged from M2), which wraps both ActivityAdapter and a given Activity:

class TurnContext {
    constructor(
        public adapter: ActivityAdapter,
        public activity: Activity
    ) {
        ...
    }

    ...

    onSendActivity(...);    // add to a pipeline that ultimately calls adapter.SendActivity
    sendActivity(...);      // calls the above mentioned pipeline

    onUpdateActivity(...);  // add to a pipeline that ultimately calls adapter.UpdateActivity
    updateActivity(...);    // calls the above mentioned pipeline

    onDeleteActivity(...);  // add to a pipeline that ultimately calls adapter.DeleteActivity
    deleteActivity(...);    // calls the above mentioned pipeline

    ...
}

This is as simple as:

...
adapter.onTurn(async activity => {
    const context = new TurnContext(adapter, activity);
    await context.sendActivity(`This durn thing works!`);
    ...
});

TurnContext and Proactive messages

It works, but it's a little painful:

adapter.onTurn(activity => {
    const context = new TurnContext(adapter, activity);
    if (context.request.type === 'message' && isRudeComment(context.request.text)) {
        await adapter.createTurnForConversation(getManagerConversationReference(context.request.user.id), async activity => {
            const context = new TurnContext(adapter, activity);
            await context.sendActivity(`Please be aware that your employee is using some salty language.`);
        });
    }
});
...

The Bot helper class, described shortly, provides a better way to do this.

Extending TurnContext

Some bot developers would like to extend TurnContext by adding useful properties and methods, often from services. By doing this they end up with a more powerful context object they can pass around to child functions. This is purely a convenience for developers who want to do this.

class MyContext <S> extends TurnContext {
    conversationState: S;

    private constructor(
        adapter: ActivityAdapter,
        activity: Activity,
    ) {
        super(adapter, activity);
    }

    async static from <S = any> (
        adapter: ActivityAdapter,
        activity: Activity,
        conversationState: ConversationState<S>,
    ) {
        const mycontext = new MyContext<S>(adapter, activity);
        mycontext.conversationState = await conversationState.read(mycontext);
        return mycontext;
    }
}

const conversationState = new ConversationState<{ count: number }>(new MemoryStorage());

adapter.onTurn(async activity => {
    const context = await MyContext.from(adapter, activity, conversationState);
    context.conversationState.count++;
    await context.sendActivity(`This durn thing works!`);
});

Bot

The Bot class (Open Issue needs a more descriptive name) is a convenience class for using context.

class Bot <Context> {
    constructor(
        ... adapters: ActivityAdapter[], 
        contextFactory?: (adapter: ActivityAdapter, activity: Activity) => Promiseable<Context>
    ) {
    }

    onTurn(handler: (context: Context) => Promise<void>): Promise<void>;

    createConversation(conversationReference: ConversationReference): Promise<ConversationReference | undefined>;    

    createTurnForConversation(conversationReference: ConversationReference, handler: (context: Context) => Promise<void>): Promise<boolean>;
}

Bot in action

const bot = new Bot(
    consoleAdapter,
);

bot.onTurn(async context => {
    await context.sendActivity(`This durn thing works!`);
});

Custom Contexts

You can provide your own context factory for custom contexts:

const bot = new Bot(
    consoleAdapter,
    (adapter, activity) => MyContext.from(adapter, activity, conversationState),
);

// here context is of type MyContext
bot.onTurn(async context => {
    context.conversationState.count++;
    await context.sendActivity(`This durn thing works!`);
});

The context factory defaults to:

(adapter, activity) => Promise.resolve(new TurnContext(adapter, activity));

Multiple Adapters

You can create a single bot that combines the listeners from multiple adapters :

const bot = new Bot(
    new SomeServiceAdapter(...),
    new AnotherServiceAdapter(...),
);

bot.onTurn(context => {
    await context.sendActivity(`This durn thing works!`);
});

Of course you can have multiple adapters and a custom context factory:

const bot = new Bot(
    new SomeServiceAdapter(...),
    new AnotherServiceAdapter(...),
    (adapter, activity) => MyContext.from(adapter, activity, conversationState),
);

bot.onTurn(context => {
    context.conversationState.count++;
    await context.sendActivity(`This durn thing works!`);
});

Bot and Proactive messages

This is nice and simple:

bot.onTurn(context => {
    if (context.request.type === 'message' && isRudeComment(context.request.text)) {
        await bot.createTurnForConversation(getManagerConversationReference(context.request.user.id), async context => {
            await context.sendActivity(`Please be aware that your employee is using some salty language.`);
        });
    }
});

Proactive messages are a little tricky with multiple adapters, because how do you know which adapter to use for a given ConversationReference? This is why adapter.createTurnForConversation returns Promise<boolean> - it's true if the adapter was able to create an activity. Bot uses this feature to implement its own createTurnForConversation method, looping through each adapter and calling its createTurnForConversation method, stopping with the first one that returns true. The result is nice and simple.

bot.createConversation(conversationReference) works similarly.

Open Issue I can imagine other approaches for accomplishing the same thing.

Middleware

Want middleware? You create your middleware pipeline independently of everything else. Nothing else in the system even knows middleware exists. It's a purely optional construct. In fact if you don't like the way this does middleware, you can roll your own.

class Pipeline <Context extends TurnContext> {
    use(middleware: Middleware | MiddlewareHandler): Pipeline;
    run(handler?: (context: Context) => Promise<void>): (context: Context) => Promise<void>;
}

const pipeline = new Pipeline()
    .use(middlewareA)
    .use(middlewareB);

bot.onTurn(pipeline.run(async context => {
    await context.sendActivity(`This durn thing works!`);
}));

You may want to run this pipeline before other code, e.g. in the "proactive" part of the salty language bot:

bot.onTurn(pipeline.run(async context => {
    if (context.request.type === 'message' && isRudeComment(context.request.text)) {
        await bot.createTurnForConversation(getManagerConversationReference(context.request.user.id), pipeline.run(async context => {
            await context.sendActivity(`Please be aware that your employee is using some salty language.`);
        }));
    }
}));

The nice thing about extracting out the middleware pipline from ActivityAdapter and Bot is that you can have multiple pipelines for different purposes (e.g. one for requests, one for proactive messages). This is also a big win for unit testing.

Obviously this separating of concerns results in more code, but again that can be wrapped by any number of abstractions. The average bot developer may only ever see, e.g.:

const bot = new ConsoleBot()
    .use(middlewareA)
    .use(middlewareB)
    .onTurn(async context => {
        if (context.request.type === 'message' && isRudeComment(context.request.text)) {
            await bot.createTurnForConversation(getManagerConversationReference(context.request.user.id), async context => {
                await context.sendActivity(`Hey friend, what do you want on your pizza?`);
            });
        }
    });

The better we separate concerns in our primitives, the more flexibility developers have to create high quality abstractions.

[drub0y]

Each revision better than the last. Biggest observations after reading through a couple times...

RE: ActivityAdapter

Love it. If I want to work at just the "raw" activity adapter level, I don't even need turn context. This aligns with what I was doing in my PoC work as well. I happen to call this abstraction a ConversationChannel instead.

RE: Custom TurnContexts

I'm still not personally sold on this. In .NET, any adapter specific functionality can be provided through a pairing of an extension method for ITurnContext and an adapter specific service that would reside in the Services collection.

In TS, I think we accomplish the same thing with declaration merging. Each framework extension library that wanted to provide extra functionality can simply enrich [I]TurnContext declaration with more methods and of course the bot developer can do the same thing just for their own bot if they want to.

RE: Proactive Messages

I like the createTurnForConversationReference concept being put on the adapter a lot.

RE: Multiple Adapters

Yaaaas.❤️

RE: Middleware

:100: This optional, pluggable approach. :100:

RE: Bot Proactive Messages

So, let me restate what I think the scenario is here and then I'll speak to it:

Once you've got multiple ActivityAdapter registered with your bot and you want to send a proactive message out to a user from an existing conversation reference, as the bot coder you aren't sure exactly which channel the reference goes to and you don't want to (read: shouldn't be) be in charge of figuring that out. So the bot's createTurnForConversationReference is actually going to take the conversation reference and "shop it around" to each adapter and the first one to return true from the underlying createTurnForConversation is the one that will actually handle that request and all other adapters will be short circuited, right?

If that's wrong, please stop reading here and correct me and I'll follow up to the correction. 😀

If that's right, I guess I just don't like the idea of having to invoke O(N) adapters just to figure out which one should process the message. Nor do I love that it overloads that method with this additional responsibility of no-op'ing if it doesn't know what to do. I'd rather see an ActivityAdatper standing on its own throw an exception if it's handed a conversation reference that it can't process.

Could we maybe register the adapters with the Bot in a way that allows the bot to route conversation references directly? Maybe ActivityAdapter itself exposes a property that lists a fixed set of identifiers it handles? Or, if this somehow needs to be completely dynamic (i.e. can change throughout the lifetime of the bot), it could be a method call that we still O(N) through I guess, but we could at least separate the "can you handle this conversation reference?" (called maybe isSupportedConversation) check from the actual createTurnForConversation method itself.

[cmayomsft]

Agree, this update makes things even more clear.

RE: ActivityAdapter and "Turn"

I like solidifying around the "Turn" naming across ActivityAdapter and Bot. For a dialog turn, I receive an Activity or TurnContext in onTurn() and I (optionally) complete the turn by replying with Activities.

RE: customizing onTurn()

From a non-FP perspective, maybe I've been looking at the V4 SDK too long, but this seems perfectly reasonable to me.

RE: TurnContext and Proactive messages

Again, this seems perfectly reasonable, especially with the simplified example.

RE: Bot

The Bot naming seems perfectly reasonable to me. Most bot developers think/talk about their bot as what receives/responds with activities to complete a turn. And, creating a custom context for my Bot from TurnContext give me by "Bot context", which is easy to grok.

RE: Bot Proactive Messages

Same question here.

If that's right, it would be great to be able to tie the conversation you want to continue or create back to the adapter you'll need.

But, at this point, that's just an optimization of this, so headed in the right direction.

[billba]

To respond to feedback from @drub0y and @cmayomsft:

naming

I'm not tied to ActivityAdapter -- I was mostly trying to evoke the M1 class. I like the name ChannelAdapter quite a bit.

custom TurnContexts

Declaration merging is just a typing construct. Someone still needs to inject properties or methods into context and we left that behind in M1. So while you certainly don't need to use custom TurnContext, if you want to pass a more powerful context around in JavaScript, I think custom TurnContext is the only way to go. (In C# you can use extension methods, which are quite different than TypeScript declaration merging.)

bot proactive messages

You got it @drub0y, and yeah this is ripe for a more efficient solution.

[GeekTrainer]

My $0.02

As I read through @billba's samples, in each situation I can intuitively figure out what is being done. It reads rather naturally, and is in many ways self documenting. In particular, multiple adapter support brought a tear to my eye.

That said, the one challenge I have, which others have pointed out, is proactive messaging. The way it's structured isn't intuitive, and with everything else being as clean as it is, it really is striking.

[billba]

one challenge I have, which others have pointed out, is proactive messaging. The way it's structured isn't intuitive

@GeekTrainer can you expand on this? It's exactly the same signature as M1, which I thought you liked:

bot.createTurnForConversation(conversationReference, context => {
    // your proactive logic goes here
});

[GeekTrainer]

@billba I didn't like it for M1. :-) I honestly found it rather confusing there as well.

The first challenge I have is createTurnForConversation, which doesn't really tell me much. (I also have a deeper challenge with the term conversation in a post dialog world, as it's not clearly defined, but that's a different conversation.) createContext is a cleaner name.

The second challenge I have is with the callback/delegate, and it's something I struggled with when doing unit testing (I can try to dig the project back up again, but I may have deleted it :-( ). I liked the cleanness of v3, where I was able to create a message, and then send it via the bot. I'd love something like

const proactiveActivity : Activity = {
  ... storedActivityWithAddressInfo
  , text: `Someone is doing something bad`
  , type: `message`
};

bot.send(proactiveActivity);

I'm not proposing the code above specifically be what's implemented, but rather the style/shape. I really would love to get away from the callback in particular.

[sgellock]

Due to the reworked Dialog and v4 SDK changes, we're going to close this. If you look at v4 and want to bring the issue up for consideration, please feel free to do so.