Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JavaScript samples incorrectly use the ActivityHandler's onMessage handler in place of the onDialog handler #1780

Closed
JonathanFingold opened this issue Sep 18, 2019 · 3 comments
Assignees

Comments

@JonathanFingold
Copy link
Contributor

JonathanFingold commented Sep 18, 2019

Sample information

  1. Sample type: [\samples\]
  2. Sample language: [es6 or nodejs or typescript]
  3. Sample name: All JS dialog samples, for instance javascript_nodejs/05.multi-turn-prompt.

Describe the bug

The code comments in the SDK clearly states that onDialog should be used to "handle Dialog activity"; however, every JS sample I've looked at splits dialog activity between the onMessage and onDialog handlers.

To Reproduce

Steps to reproduce the behavior:

  1. Look at the code comments for onDialog:
    /**
     * onDialog fires at the end of the event emission process, and should be used to handle Dialog activity.
     * @remarks
     * ...
     * @param handler BotHandler A handler function in the form async(context, next) => { ... }
     */
     public onDialog(handler: BotHandler): this {
         return this.on('Dialog', handler);
     }
    https://github.com/microsoft/botbuilder-js/blob/73990aee9aad1b54ba3941d284b9aec3d0009c5f/libraries/botbuilder-core/src/activityHandler.ts#L176-L194
  2. Look at a typical implementation of a JS bot that extends the ActivityHandler:
    this.onMessage(async (context, next) => {
    console.log('Running dialog with Message Activity.');
    // Run the Dialog with the new message Activity.
    await this.dialog.run(context, this.dialogState);
    await next();
    });
    this.onDialog(async (context, next) => {
    // Save any state changes. The load happened during the execution of the Dialog.
    await this.conversationState.saveChanges(context, false);
    await this.userState.saveChanges(context, false);
    await next();
    });
  3. Error, these two patterns are nigh-incompatible.

Expected behavior

Our official samples follow our own best practices.

Additional context

Using onMessage as the "jumping off point" for beginning or continuing a dialog is problematic for dialogs that need to handle authentication (event activity) or ones that might welcome a user to the conversation (conversationUpdate activity).

A similar issue exists in the C# samples, but we have no corresponding guidance to use the OnTurn handler after it's inner call to next completes.

[bug]

@EricDahlvang
Copy link
Member

@stevengum has a solution for this:

#1964

@stevengum
Copy link
Member

One additional scenario where we don't use onDialog for dialogs but should, is for bots using the ActivityPrompt. ActivityPrompt was created with the idea that a user might not necessarily respond to a prompt with a "Message"-type activity. They might instead perform an event that the bot interprets as a response to a the prompt.

E.g. A voice-enabled bot in a car asks the driver to put on their seatbelt. When the driver puts on their seatbelt, the bot should not wait for the driver to say "my seatbelt is on". The car should send an "Event"-type activity to the bot once the seatbelt is engaged, indicating that the driver responded to the prompt.

@johnataylor
Copy link
Member

We've updated the samples - closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants