Botbuilder-js: idea: consolidate 3 middleware pipes into 1

[Stevenic]

To begin with this idea is not trying to address typing and context mucking issues raised by @billba in issue #56. This is to address an issue I've ran into several times while implementing middleware for the v4 SDK.

An issue I keep running into is the need to pass some information from one middleware pipe like contextCreated() to another pipe like sendActivity. Take this simple logger plugin below:

export class MyLogger implements Middleware {
    static id = 0;
    private key: string;

    constructor() {
        this.key = 'myLogger:' + MyLogger.id++;
    }

    public contextCreated(ctx, next) {
        const logger = new Logger();
        ctx[this.key] = logger;
        logger.log(`started`);
        return next()
            .then(() => logger.log(`ended`));
    }

    public sendActivity(ctx, activities, next) {
        const logger = ctx[this.key];
        logger.log(`sending`);
        return next()
            .then(() => logger.log(`sent`));
    }
}

bot.use(new MyLogger());

I want to log all activity flowing into an out of the bot so in contextCreated() I create my logger and all is well. The problem is I want to use the same logger when logging outgoing activities in sendActivity() so I some how need it get it from closure where I created it to this other closure. Short of caching it in a map my only other option is to tack it onto the context object and let it haul it over to the other closure for me. To avoid potential collisions I need to essentially use a GUID to store it which I create when the plugin is created.

The whole thing feels pretty gross and I wouldn't have to do this at all if everything was under the umbrella of a single closure. We still need the separate pipes though so what would that look like?

bot.use((ctx, next) => {
    const logger = new Logger();
    logger.log(`started`);

    ctx.onSend((ctx, next) => {
        logger.log(`sending`);
        return next()
            .then(() => logger.log(`sent`));
    });

    return next()
        .then(() => logger.log(`ended`));
});

This looks a lot more like a traditional piece of middleware you'd define in Express or Redux but there's no loss of functionality. A single outer closure now replaces the contextCreated pipe but functionally it works the same way. The key difference is that where the other pipe like receiveActivity and sendActivity were statically defined before as part of the middleware registration they now get dynamically registered with the context object at runtime which makes them look a lot like events.

This approach lets the other pipes run under the hood of the outer closure and lets us use closure scope to hold shared things like the logger. As you can see the code is simplified quite a bit from the original example.

To see a more complete example lets look at an entire processing stack built out using this new approach:

const storage = new MemoryStorage();
const bot = new Bot(adapter)
    // Log incoming and outgoing activities
    .use((ctx, next) => {
        console.log(`begin turn.`)

        ctx.onSend((ctx, activities, next) => {
            console.log(`sending activities.`);
            return next();
        });

        return next()
            .then(() => console.log(`end of turn`), (err) => console.error(`error occurred`))
    })

    // Report an errors thrown to the user
    .use((ctx, next) => {
        return next()
            .catch((err) => {
                return ctx.send(`An error occurred`).then(() => err);
            });
    })

    // Read/write bots conversation state
    .use((ctx, next) => {
        const key = 'convo:' + ctx.reequest.conversation.id;
        return storage.read(key)
            .then((state) => ctx.state = state)
            .then(() => next())
            .then(() => storage.write(key, state));
    })

    // Route received activities through bots logic.
    .onReceive((ctx) => {
        ctx.state.count = (ctx.state.count || 0) + 1;
        return ctx.send(`${ctx.state.count}: You said: ${ctx.request.text}`);
    });

This bots stack first uses a piece of logging middleware which will log all incoming and outgoing activities, next an error reporter watches for an errors and notifies the user, then our bot state middleware runs, and finally the bots logic. When the bots logic completes everything unwinds back up the stack and even though there's no change in functionality it feels less like we're making multiple passes through the stack because now we're just calling what feel like event subscribers.

Bonus is that other parts of your bot logic can subscribe to send notifications as well.

[billba]

I'm warming to this idea.

I don't like the idea of hanging on* on context, because people will inevitably try calling them outside of the middleware pipeline. Since they are only really relevant in middleware, how about something like:

bot.use(async (ctx, next, handlers) => {
    console.log(`Start of turn`);
    handlers.onSend(async (ctx, next) => {
        console.log("sending");
        await next();
        console.log("sent");
    });
    await next();
    console.log(`End of turn`);
});

[Stevenic]

Why wouldn't you be able to register a listner outside of middleware? That seems like one of the advantages. If something in my bot logic wants to modify outgoing posts it should be able to

[Stevenic]

For what it's worth this cleans up the execution of middleware quite nicely. There was this weird dance that was happening where anytime you sent an activity, some parts of the logic would execute on the bot side of things and other parts would happen on the context side.

With this particular change now when you send() all processing logic happens on the context side of things and the Bot (what's now called the BotAdapter) has 3 very clean methods for sending, updating, and deleting activities. If you call these 3 methods on the context object anyone subscribed for notifications will get notified prior to execution of the operation. If you want to bypass that just use context.adapter and call the exact same 3 methods there. It's super clean now.

[garypretty]

I like this approach and I personally don't see a problem with having the ability to register listeners on the context. Certainly the overall approach looks much cleaner and easier to understand.

[billba]

I thought about this some more and I changed my mind. I love it. Being able to attach handlers in onReceive means much more flexibility in where stuff goes. It doesn't have to go in middleware.

[billba]

On that note, I understand that the name use comes from Express, but if we're back to a single function for our middleware then I much prefer the descriptive name onTurn:

bot
    .onTurn((turn, next) => { ... })
    .onRequest(turn => { ... })

This spells it out. onTurn is executed on every turn (request and proactive) and onRequest is executed on every request.

The last piece would be making the proactive case more descriptive. How about createTurn:

bot.createTurn(turn => { ... })

The nice thing about this is that the shared naming gives you a strong hint that onTurn will be called.

[billba]

I would further like to suggest that .use() (or .onTurn() or whatever we end up calling it) take either a function or an object that exposes a specific named function, e.g. myObject.onTurn(context, next). That allows you to have a hybrid object (e.g. StateManager) that acts both as a service and as middleware. Extremely useful.

[Stevenic]

@billba I've already added the ability to pass in a function or object. Good suggestion.

[billba]

What's the annointed name for the method in the object?

[Stevenic]

@billba it's called onProcessRequest to align with the new adapter.processRequest() method you call to route an incoming request.

[billba]

What I don't like about that is that it's not aligned with the method used to create proactive turns. I would infer that it only applies to the request loop, not proactive turns.

As mentioned above, I think a better approach is to share the name with the way the middleware is invoked. Currently that would be use. I still greatly prefer onTurn as a much more descriptive name for both those things.

[Stevenic]

There are 2 different methods used to start proactive convos. I'm not crazy about any of the names I thought of so I'm open to other names. Could be onTurn but honestly I'm not crazy about that either. onRoute could be another option. @cleemullins or @tomlm any thoughts?

[billba]

Then that's even worse because if there are three times that the middleware pipeline is invoked, and your current name is shared with only one of them, it sends a strong message that it's not being invoked for the other two. If our naming can't lead them towards the right answer, let's at least avoid leading them away from it.

[Stevenic]

done