Add CancelInvocation support to MsgPack in TS client #7224
Conversation
src/SignalR/clients/ts/signalr-protocol-msgpack/tests/MessagePackHubProtocol.test.ts
Outdated
Show resolved
Hide resolved
…ackHubProtocol.test.ts Co-Authored-By: BrennanConroy <[email protected]>
|
Unfortunately there is no good way in the functional tests to test that it actually did anything. Unless I do some ugly stuff with using the |
Is the "can stream server method and cancel stream" test not verifying this change actually did something? |
|
It is, but I had to do the |
|
Fair. It wasn't that ugly. |
Opinions |
|
🆙 📅 |
| const subject = new Subject<T>(); | ||
| subject.cancelCallback = () => { | ||
| const cancelInvocation: CancelInvocationMessage = this.createCancelInvocation(invocationDescriptor.invocationId); | ||
|
|
||
| delete this.callbacks[invocationDescriptor.invocationId]; | ||
|
|
||
| return this.sendWithProtocol(cancelInvocation); | ||
| return promiseQueue.then(() => { | ||
| return this.sendWithProtocol(cancelInvocation); |
There was a problem hiding this comment.
I assume this failed before if you canceled the stream too early. Do we have any regression tests the promiseQueue part of this change?
There was a problem hiding this comment.
The promiseQueue doesn't change with this PR, I just use it somewhere I didn't before.
The test change needed was https://github.com/aspnet/AspNetCore/pull/7224/files#diff-e53c0bba1752f4416da08f9f40b4ac92R1078 to wait for a message since it now goes async.
There was a problem hiding this comment.
I know that the promiseQueue existed before. Are you saying that without this change, the "automatically sends multiple pings" would fail? Do you think that's a reliable regression test for this?
There was a problem hiding this comment.
The "stream can be canceled" test would fail because TS isn't threaded, so the cancel message wouldn't be sent before getting to the expect(connection.sentData.length).toBe(3); check. Now the cancel callback waits until the stream has finished being started before allowing the cancel to be sent.
I think the test is good.
Fixes #7157