Skip to content

Conversation

@Zerryth
Copy link
Contributor

@Zerryth Zerryth commented Feb 20, 2020

Fixes #1663

Description

Issue: In the NamedPipeServer we have direct type dependencies on classes from the "net" module. We should use the library abstractions to avoid unclear type dependencies on Node.js builtins.

Specific Changes

Similar to work done to remove "dom" from bf-streaming tsconfig and code refactoring, added the following:

  • Create INodeServer interface for usage of what was node's Sever class from 'net' module in our framework's NamedPipeServer
  • Add helper methods to create node net Server without relying on "net" being in the tsconfig.json.

@Zerryth
Copy link
Contributor Author

Zerryth commented Feb 20, 2020

Still work in progress -- tests not passing...

@coveralls
Copy link

coveralls commented Feb 20, 2020

Pull Request Test Coverage Report for Build 107356

  • 8 of 10 (80.0%) changed or added relevant lines in 3 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 73.003%

Changes Missing Coverage Covered Lines Changed/Added Lines %
libraries/botframework-streaming/src/namedPipe/namedPipeServer.ts 6 8 75.0%
Files with Coverage Reduction New Missed Lines %
libraries/botframework-streaming/src/namedPipe/namedPipeServer.ts 4 79.78%
Totals Coverage Status
Change from base Build 107271: -0.03%
Covered Lines: 8619
Relevant Lines: 11278

💛 - Coveralls

@Zerryth
Copy link
Contributor Author

Zerryth commented Feb 20, 2020

Notes to self:

It breaks right now, because the only way for NamedPipeServer to create a Server right now is if Server is in the global scope.

In order to make this work, might need to build a NodeNetServer class similar to NodeWebSocket.

That way, as in WebSocketClient.connect(), there is a fallback--if there's a global one, create a Server using NamedPipeServer.createServer(), otherwise initialize our NodeNetServer .


-- the tests on BrowserWebSocket don't break when testing its connect() method, because we never test the BrowserWebSocket.connect() path when BrowserWebSocket.socket is undefined (and would therefore try to create one from global WebSocket class)

  • even if we did try to test this, we would probably test that it throws, since there isn't a global WebSocket or be forced to create a "valid" WebSocket with a connect() method and add it to global

---however the tests for NamedPipeServer do break, because we are heading down the path of needing a server, and the only server we make available is if it's available in global

@Zerryth Zerryth closed this Feb 26, 2020
@Zerryth Zerryth deleted the Zerryth/NamedPipe-removeDependencies branch February 26, 2020 03:34
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.

[Streaming] Stop using @types/node in botframework-streaming

3 participants