-
Notifications
You must be signed in to change notification settings - Fork 127
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
Ignore errorConversationEnded on end() #133
Conversation
@ckkashyap Please review this. |
Hi @orgads. Thank you for the pull request. Can you provide some information in the description explaining the motivations for the change and include steps to reproduce the error you're trying to avoid? Thank you. |
It is expected to be thrown there. Example: directLine.activity$ .filter(activity => activity.type === 'message') .subscribe( message => { console.log("received message ", message) directLine.end(); } ); /.../node_modules/botframework-directlinejs/node_modules/rxjs/Subscriber.js:246 throw err; ^ Error: conversation ended at Object.<anonymous> (/.../node_modules/botframework-directlinejs/built/directLine.js:47:30) at Module._compile (internal/modules/cjs/loader.js:689:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10) at Module.load (internal/modules/cjs/loader.js:599:32) at tryModuleLoad (internal/modules/cjs/loader.js:538:12) at Function.Module._load (internal/modules/cjs/loader.js:530:3) at Module.require (internal/modules/cjs/loader.js:637:17) at require (internal/modules/cjs/helpers.js:22:18) at Object.<anonymous> (/.../azure-bot.js:3:42) at Module._compile (internal/modules/cjs/loader.js:686:14)
I added an example to the commit message.
|
ping? |
@orgads is it possible to inspect the the state of the connectionStatus observer to guard the call to Something like if (this.connectionStatus$.getValue() !== ConnectionStatus.Ended) {
this.connectionStatus$.next(ConnectionStatus.Ended);
} |
What do you mean? |
I edited my comment with an example of what I was thinking. |
Oh, I see. I'll test it. |
Doesn't help. You can easily try it yourself. I suppose you have a key. This is my test app:
|
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.
@orgads thanks for your patience as we discuss this offline. We think that there may be a more comprehensive fix to this issue, but we plan on doing a major refactor away from rxjs in the future so this fix will suffice.
We will get this merged and published later today.
Thank you. |
It is expected to be thrown there.