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

Ignore errorConversationEnded on end() #133

Merged
merged 1 commit into from
Jan 25, 2019
Merged

Conversation

orgads
Copy link
Contributor

@orgads orgads commented Dec 24, 2018

It is expected to be thrown there.

@orgads
Copy link
Contributor Author

orgads commented Dec 24, 2018

@ckkashyap Please review this.

@cwhitten
Copy link
Member

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)
@orgads
Copy link
Contributor Author

orgads commented Jan 12, 2019

I added an example to the commit message.

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)

@orgads
Copy link
Contributor Author

orgads commented Jan 23, 2019

ping?

@cwhitten
Copy link
Member

Hi @orgads. We're evaluating the side effects of this change (and the issue you are trying to address with it) in our WebChat project here. We hope to land this soon. Thank you.

@cwhitten
Copy link
Member

cwhitten commented Jan 25, 2019

@orgads is it possible to inspect the the state of the connectionStatus observer to guard the call to next(...) instead of wrapping the call to next(...) in a try/catch?

Something like

if (this.connectionStatus$.getValue() !== ConnectionStatus.Ended) {
  this.connectionStatus$.next(ConnectionStatus.Ended);
}

@orgads
Copy link
Contributor Author

orgads commented Jan 25, 2019

What do you mean?

@cwhitten
Copy link
Member

I edited my comment with an example of what I was thinking.

@orgads
Copy link
Contributor Author

orgads commented Jan 25, 2019

Oh, I see. I'll test it.

@orgads
Copy link
Contributor Author

orgads commented Jan 25, 2019

Doesn't help. You can easily try it yourself. I suppose you have a key. This is my test app:

global.XMLHttpRequest = require('xhr2');
global.WebSocket = require('ws');
const { DirectLine } = require('./built/directLine.js');
var directLine = new DirectLine({ secret: "<key>" });
directLine.activity$
.filter(activity => activity.type === 'message')
.subscribe(
    message => { console.log('ending...'); directLine.end(); }
);

@a-b-r-o-w-n a-b-r-o-w-n self-requested a review January 25, 2019 17:14
Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n left a 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.

@a-b-r-o-w-n a-b-r-o-w-n merged commit 544e7e3 into microsoft:master Jan 25, 2019
@orgads
Copy link
Contributor Author

orgads commented Jan 26, 2019

Thank you.

@orgads orgads deleted the end-ex branch January 26, 2019 16:35
@a-b-r-o-w-n
Copy link
Contributor

@orgads 0.11.0 has been published to npm.

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.

3 participants