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

cleanup: middleware handling #2951

Merged
merged 4 commits into from
Oct 30, 2020
Merged

Conversation

joshgummersall
Copy link
Contributor

  • Rewrite middleware handling logic in async/await
  • Add middleware tests for various error scenarios
  • Fix issue in ShowTypingMiddleware where a promise rejection would yield never-ending typing activities
  • Remove unnecessary prototype references

These changes are all a precursor to the immediateAccept change. The rewritten code was all an effort to make the runMiddleware chain easier to read through. I introduced extensive middleware testing before making changes to ensure there are no new issues introduced.

@joshgummersall joshgummersall changed the title Jpg/refactor middleware set cleanup: middleware handling Oct 23, 2020
@joshgummersall joshgummersall force-pushed the jpg/refactor-middleware-set branch 2 times, most recently from f93e8bc to ba38eba Compare October 26, 2020 21:51
@joshgummersall joshgummersall force-pushed the jpg/refactor-middleware-set branch from ba38eba to 64c5334 Compare October 27, 2020 19:36
Comment on lines 108 to +109
} else if (typeof plugin === 'object' && plugin.onTurn) {
this.middleware.push((context: TurnContext, next: Function) => plugin.onTurn(context, next));
this.middleware.push((context, next) => plugin.onTurn(context, next));
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if we'll ever get to this, but I'd like to get your input and formalize the JS SDKs expectation for complex parameters that exist as implementations in the SDK.

We communicate to developers that they have to consume all BF SDK dependencies in lock-step, mostly because that's the extent of our test matrix (packages are tested against the same version)... But also because if a bot consumes two different versions of a given botbuilder-* package, it may blow up due to instanceof misalignment. And in TypeScript scenarios, the project might not build due to ambiguous types due to multiple versions of a botbuilder-* package.

Copy link
Member

Choose a reason for hiding this comment

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

And my last but favorite point, interfaces have different semantics between C# and TypeScript at runtime. And Python doesn't have interfaces at all (abc).

Copy link
Member

@stevengum stevengum left a comment

Choose a reason for hiding this comment

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

:shipit:

@joshgummersall joshgummersall force-pushed the jpg/refactor-middleware-set branch from 390b0bd to 20f0f39 Compare October 30, 2020 17:24
@joshgummersall joshgummersall force-pushed the jpg/refactor-middleware-set branch from 20f0f39 to 543336d Compare October 30, 2020 17:29
@joshgummersall joshgummersall merged commit d6088e2 into main Oct 30, 2020
@joshgummersall joshgummersall deleted the jpg/refactor-middleware-set branch October 30, 2020 17:57
joshgummersall added a commit that referenced this pull request Nov 30, 2020
* Overhaul middlewareSet tests

* Clean up middleware handling logic

* Expand BotAdapter tests with error handling tests

Co-authored-by: Steven Gum <[email protected]>
stevengum added a commit that referenced this pull request Dec 7, 2020
* Overhaul middlewareSet tests

* Clean up middleware handling logic

* Expand BotAdapter tests with error handling tests

Co-authored-by: Steven Gum <[email protected]>

Co-authored-by: Steven Gum <[email protected]>
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.

2 participants