Feedback middleware suggestions

[Stevenic]

I have a couple of suggestions for improving your Feedback middleware. The primary one being that you should pass a StatePropertyAccessor to your middlewares constructor instead of a ConversationState object. This will eliminate the need to extend the TurnContext like you're doing and generally simplify working with your feedback record. So your middlewares constructor and onTurn() should look something like this:

export class FeedbackMiddleware implements Middleware {
    constructor(private feedbackProp: StatePropertyAccessor<FeedbackRecord>, private options?: FeedbackOptions) { }

    async onTurn(context: TurnContext, next: () => Promise<void>): Promise<void> {
        // Get feedback record
        const record = await this.feedbackProp.get(context);
        if (record) {
            // ... do something ...
        }
    }
}

You can call await this.feedbackProp.set(context, record) to initialize the record and await this.feedbackProp.delete(context) to clear it.

And if you want to save the property accessor on your TurnContext so that you can access it from your static methods you should use the TurnContext.turnState map instead of extending the context object. Updating the sample above you would do this:

const feedbackKey = Symbol('feedback');

export class FeedbackMiddleware implements Middleware {
    constructor(private feedbackProp: StatePropertyAccessor<FeedbackRecord>, private options?: FeedbackOptions) { }

    async onTurn(context: TurnContext, next: () => Promise<void>): Promise<void> {
        // Save feedback prop to turn context
        context.turnState.set(FeedbackKey, this.feedbackProp);

        // Get feedback record
        const record = await context.turnState.get(feedbackKey).get(context);
        if (record) {
            // ... do something ...
        }
    }
}

The Symbol() just helps ensure that you don't have naming collisions with other components caching stuff to the TurnContext.

[ctstone]

@Stevenic, thanks for the info on the new SDK components.

I like the addition of context.turnState, and that should eliminate the need for me to store the conversation state accessor (or anything else) directly on the context object.

However, I'm a little unclear on the use of StatePropertyAccessor<T>. Reviewing the sample, I can see that someone still needs to create this object by calling conversationState.createProperty('some-string'). Wouldn't it make sense for me to continue accepting a ConversationState in my constructor, and then creating my own named/typed StatePropertyAccessor<FeedbackRecord> when needed? Otherwise, users of this middleware plugin have to do that work themselves, and it's not clear that there is a benefit to exposing that complexity to them.

I like the use of symbols to store map values, but conversationState.createProperty(..) only accepts a string. Is this because the value will eventually have to be serialized?