fix: upgrader should not need muxers #517
Conversation
| ]) | ||
|
|
||
| expect(connections).to.have.length(2) | ||
| }) |
There was a problem hiding this comment.
Should I test the errors with multiplexer operations in the connection?
There was a problem hiding this comment.
It might be good to add a quick test. They should also throw errors with codes.
There was a problem hiding this comment.
I think it would be good to also add a test for closing. It's going to the MultiaddrConn so it should be fine, but I think having it for avoid regressions in the future would be good.
There was a problem hiding this comment.
I added to this test. I think it is better than create a new test
vasco-santos
force-pushed
the
fix/upgrader-should-not-need-muxers
branch
from
45c2760
to
8d7c110
Compare
| ]) | ||
|
|
||
| expect(connections).to.have.length(2) | ||
| }) |
There was a problem hiding this comment.
It might be good to add a quick test. They should also throw errors with codes.
vasco-santos
force-pushed
the
fix/upgrader-should-not-need-muxers
branch
from
1e7dc25
to
7eb4309
Compare
| @@ -244,20 +292,9 @@ class Upgrader { | |||
| } | |||
|
|
|||
| // Pipe all data through the muxer | |||
| pipe(muxedConnection, muxer, muxedConnection) | |||
| pipe(upgradedConn, muxer, upgradedConn) | |||
|
|
|||
| maConn.timeline.upgraded = Date.now() | |||
There was a problem hiding this comment.
We should just set this right after we created the timeline Proxy, that way we don't need to set it here and in the !Muxer if statement above.
| ]) | ||
|
|
||
| expect(connections).to.have.length(2) | ||
| }) |
There was a problem hiding this comment.
I think it would be good to also add a test for closing. It's going to the MultiaddrConn so it should be fine, but I think having it for avoid regressions in the future would be good.
Co-Authored-By: Jacob Heun <jacobheun@gmail.com>
vasco-santos
force-pushed
the
fix/upgrader-should-not-need-muxers
branch
from
031d6aa
to
def7e54
Compare
* fix: upgrader should not need muxers * chore: address review * chore: apply suggestions from code review Co-Authored-By: Jacob Heun <jacobheun@gmail.com>
With the current codebase, having a
streamMuxeris mandatory, which was not the case before. This PR allows a connection to be upgraded just with crypto.Note: the
connectionproperties regarding multiplexers are throwing an error now.