-
Notifications
You must be signed in to change notification settings - Fork 281
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
Streaming: refactor streaming into adapter. General improvements and quality. #1306
Conversation
…Socket and useNamedPipe methods
Pull Request Test Coverage Report for Build 84085
💛 - Coveralls |
"@types/node": "^10.12.18", | ||
"botbuilder-core": "4.1.6", | ||
"botframework-connector": "4.1.6", | ||
"botframework-streaming": "4.1.6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused why Master has v4.1.6 for all packages, are they really that far out of date or is there something special about the value?
Technically we only support Streaming for BotBuilder 4.5 or higher, but I've tested with the 4.1.6 labeled packages in Master and everything works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just stayed consistent with the rest of packages for now. What version should we ref? ^4.5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will work with 4.1.6, we just don't technically support it. I think our options are:
1 - Update the rest of the packages to the current release versions.
2 - Go with 4.1.6 for consistency.
3 - Go with ^4.5 with the knowledge it creates a bad experience for developers trying to build off of master.
@@ -136,6 +154,11 @@ const USER_AGENT: string = `Microsoft-BotFramework/3.1 BotBuilder/${ pjson.versi | |||
const OAUTH_ENDPOINT = 'https://api.botframework.com'; | |||
const US_GOV_OAUTH_ENDPOINT = 'https://api.botframework.azure.us'; | |||
const INVOKE_RESPONSE_KEY: symbol = Symbol('invokeResponse'); | |||
const defaultPipeName = 'bfv4.pipes'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need type identifiers here? :string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! It feels great to see the adapter merged down and the interim package removed. Thank you for getting this over the finish line, Carlos!
* @param pipeName The name of the named pipe to use when creating the server. | ||
* @param logic The logic that will handle incoming requests. | ||
*/ | ||
public async useNamedPipe(pipeName: string = defaultPipeName, logic: (context: TurnContext) => Promise<any>): Promise<void>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reentrancy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider warning + override
throw new Error('Streaming logic needs to be provided to `useWebSocket`'); | ||
} | ||
|
||
this.streamingLogic = logic; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cardinality, simulateneous socket + named pipe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider map based
|
||
const upgrade = res.claimUpgrade(); | ||
const ws = new Watershed(); | ||
const socket = ws.accept(req, upgrade.socket, upgrade.head); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
virtual accept for people t manage their own socket
… to determine if a Request has been passed in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial feedback
…it will need to be there forever. duplicating for now. We'll unify in 4.7 along with other refactors
…soft/botbuilder-js into ccastro/streaming-refactor
…soft/botbuilder-js into ccastro/streaming-refactor
…soft/botbuilder-js into ccastro/streaming-refactor
Fixes #1178
Description
The high level purposes of this change are mainly:
Usage
Usage: Http only bot
Usage: WebSockets bot
Specific Changes
Testing
We have 4 sources of testing
Code Coverage Report[1]
[1] Source