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

Update sample 21 to align with core bot #2219

Merged
merged 5 commits into from
Apr 14, 2020
Merged

Conversation

garypretty
Copy link
Contributor

Fixes #2203

  • Aligns core bot to core bot sample
  • Adds telemetry client to LUIS recognizer to align with C# example

@garypretty garypretty requested a review from stevengum February 28, 2020 11:31
@garypretty garypretty requested a review from Stevenic March 19, 2020 10:46
Copy link
Contributor

@WashingtonKayaker WashingtonKayaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add the WebSocket code to the end of Index.js (See PR 2065 and PR 2086):

// Listen for Upgrade requests for Streaming.
server.on('upgrade', (req, socket, head) => {
    // Create an adapter scoped to this WebSocket connection to allow storing session data.
    const streamingAdapter = new BotFrameworkAdapter({
        appId: process.env.MicrosoftAppId,
        appPassword: process.env.MicrosoftAppPassword
    });
    // Set onTurnError for the BotFrameworkAdapter created for each connection.
    streamingAdapter.onTurnError = onTurnErrorHandler;

    streamingAdapter.useWebSocket(req, socket, head, async (context) => {
        // After connecting via WebSocket, run this logic for every request sent over
        // the WebSocket connection.
        await bot.run(context);
    });
});

Copy link
Contributor

@WashingtonKayaker WashingtonKayaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor issue on line 65 of Index.js, the second parameter for TelemetryInitializerMiddleware defaults to true, so does not need to be set, and isn't set in the C# version (which is a bit of a different case due to DI, so parity may not be as important here).

@garypretty
Copy link
Contributor Author

Minor issue on line 65 of Index.js, the second parameter for TelemetryInitializerMiddleware defaults to true, so does not need to be set, and isn't set in the C# version (which is a bit of a different case due to DI, so parity may not be as important here).

@WashingtonKayaker I have removed the second param and also made the other amends you requested. Thanks for the review.

Copy link
Contributor

@WashingtonKayaker WashingtonKayaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to remove the local from the URL links.

garypretty and others added 3 commits March 26, 2020 15:11
…htBookingRecognizer.js

Co-Authored-By: WashingtonKayaker <52054121+WashingtonKayaker@users.noreply.github.com>
…es/welcomeCard.json

Co-Authored-By: WashingtonKayaker <52054121+WashingtonKayaker@users.noreply.github.com>
…es/welcomeCard.json

Co-Authored-By: WashingtonKayaker <52054121+WashingtonKayaker@users.noreply.github.com>
@garypretty garypretty merged commit ac6eeaa into master Apr 14, 2020
@cleemullins cleemullins deleted the gary/app-insights-updates branch April 22, 2020 19:38
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

Successfully merging this pull request may close these issues.

Some changes are needed to 21.corebot-app-insights
2 participants