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

Adding our own partial versions of node's 'net' Server and Socket classes #1756

Closed
wants to merge 2 commits into from

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
2 participants