Fix cleaning send queue on restart #18511
Conversation
| tryAddBrowser("ChromiumHeadlessIgnoreCert", new ChromiumHeadlessBrowser(() => { }, {})); | ||
| if (!tryAddBrowser("FirefoxHeadless", new FirefoxHeadlessBrowser(0, () => { }, {}))) { | ||
| tryAddBrowser("FirefoxDeveloperHeadless", new FirefoxDeveloperHeadlessBrowser(0, () => { }, {})); | ||
| tryAddBrowser("ChromeHeadlessNoSandbox", ChromeHeadlessBrowser.prototype); |
There was a problem hiding this comment.
This is part of updating the firefox launcher. We shouldn't have been instantiating these classes to check if the browsers exists.
| tryAddBrowser("Edge", new EdgeBrowser(() => { }, { create() { } })); | ||
| tryAddBrowser("IE", new IEBrowser(() => { }, { create() { } }, {})); | ||
| tryAddBrowser("Safari", new SafariBrowser(() => { }, {})); | ||
| tryAddBrowser("Edge", EdgeBrowser.prototype); |
There was a problem hiding this comment.
Which edge is this testing? Legacy?
There was a problem hiding this comment.
TBH we need to do some work to understand what Karma does locally and what Sauce Labs will do in the cloud. I'll file a bug for that.
For now, I'd expect yes, it's Edge Legacy.
| @@ -43,21 +43,6 @@ | |||
| resolved "https://registry.yarnpkg.com/@types/node/-/node-9.6.49.tgz#ab4df6e505db088882c8ce5417ae0bc8cbb7a8a6" | |||
| integrity sha512-YY0Okyn4QXC4ugJI+Kng5iWjK8A6eIHiQVaGIhJkyn0YL6Iqo0E0tBC8BuhvYcBK87vykBijM5FtMnCqaa5anA== | |||
|
|
|||
| "@types/strip-bom@^3.0.0": | |||
There was a problem hiding this comment.
I assume this is expected?
There was a problem hiding this comment.
Are you referring to the file changing? Yes, I updated dependencies so this file gets updated.
| return this._onopen!; | ||
| return (e) => { | ||
| this._onopen!(e); | ||
| this.readyState = this.OPEN; |
There was a problem hiding this comment.
Where is readyState used?
There was a problem hiding this comment.
Used by the WebSocketTransport
| this.webSocket.onerror = () => {}; | ||
| this.webSocket.close(); | ||
| this.webSocket = undefined; | ||
| this.cleanup(); |
There was a problem hiding this comment.
Nit: close() also calls cleanup(), and close() is called immediately after this. I'd just remove cleanup() and put the logic directly in close().
| }); | ||
| this.sendQueue = undefined; | ||
| } | ||
|
|
There was a problem hiding this comment.
While we're cleaning up random things, can we move the this.connectionStarted = false; above the if (this.onclose && this.connectionStarted) {? I wrote it that way, but don't know why. It shouldn't be dependent on whether the onclose callback was hooked. Thankfully, HubConnection always does hook the onclose callback, but still.
There was a problem hiding this comment.
I moved the if (this.onclose) check instead. We don't want to run the onclose function if the connection never started.
There was a problem hiding this comment.
We don't want to run the onclose function if the connection never started.
Yeah. That's the whole point of that logic. We don't need to check if connectionStarted is true in order to set it to false. Both ways work, but it does seem a little weird to put the if block inside the try block.
Fixes #17613
Also slipped in a couple other minor fixups:
karma-firefox-launcherso that it would actually close the browser on test exit (I had dozens of firefox instances open from running the functional tests a bunch of times)NodeHttpClientto fix a race between the LongPolling transport being closed and re-polling. (the race would only show up in tests since product code uses theDefaultHttpClientwhich already has the check)WebSocketTransportso we actually close the connection due to an "internal error". Such as throwing while parsing a message from the server.