-
Notifications
You must be signed in to change notification settings - Fork 276
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
fix: streaming client should connect under Node.js #4413
Conversation
Pull Request Test Coverage Report for Build 4003071240
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at NodeWebSocket.ts in detail and it looks fine.
@ckkashyap @compulim What is the expected impact of this being a breaking change? What would be a shame is to correct one issue and break another set of customers. |
I am modifying the API to become
As we do not have any existing tests that proves the existing code is working, we are doing our best efforts to make it backward compatible. |
I tried to write a test to show how previous signature would work. However, there are obvious mistakes in the code that made it very unlikely to work:
I am keeping the old code as-is, without any tests. |
ws.once('open', () => resolve()); | ||
}); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A warning here may be useful?
* Fix NodeWebSocket.connect * Fix ESLint * Update API * Remove .only * Clean up code related to watershed * Fix ESLint * Fix tests for Node.js 12 * Fix ESLint * Add mocks * Remove .only * Use LF * Detect older signature * Fix ESLint * serverAddress become serverAddressOrHostName * Update API * Clean up * More clean up * Clean up * Leaving old code * Switching branch * Clean up * Clean up * Clean up * Manual edit
This reverts commit 6c10061.
* Fix NodeWebSocket.connect * Fix ESLint * Update API * Remove .only * Clean up code related to watershed * Fix ESLint * Fix tests for Node.js 12 * Fix ESLint * Add mocks * Remove .only * Use LF * Detect older signature * Fix ESLint * serverAddress become serverAddressOrHostName * Update API * Clean up * More clean up * Clean up * Leaving old code * Switching branch * Clean up * Clean up * Clean up * Manual edit
Fixes #4412.
Description
NodeWebSocket.connect()
is not implemented correctly and should never work. There are multiple problems in the original implementation:WebSocket
object out, which is probably not workingclose
event for connection to established, which is wrongWe are not sure why the original implementation has done this way.
We are updating the implementation to use
ws
package to connect.Designs
Removing second argument (
port = 8082
)We are removing the second argument (
port = 8082
) because:serverAddress
will contain the port number (by default,ws:
is 80, andwss:
is 443)serverAddress
becauseport
is default to 8082This does not sound intuitive to developer because the port number in
serverAddress
will always get replaced.If we don't default port number to 8082, we will need to change the API saying there is no default (i.e.
undefined
). At the end of the day, we are still modifying the API and will introduce breaking changes.Regardless of whether we keep
port
or not, we are still introducing breaking changes. Thus, we are choosing to removeport
to simplify our code.If the developer need to change the port to
8082
, they could use the following code snippet:Specific Changes
❗This PR will introduce breaking changes and modified the API. Please bump version appropriately.
NodeWebSocket.connect()
to connect to a Web Socket server as a clientconnect()
rejections) because it will print errors to console and there is no easy way to suppresswatershed
watershed
was one of the candidate packages for using Web Socket in Node.jsws
binary
,data
, andtext
event in thews
version of Web SocketFauxSock
which mocks Web Socket and TCP/IP Socket, we are opting for a more specific mock for TCP/IP Socket, namelyFauxSocket
Testing
Added tests to ensure
NodeWebSocket
can connect to a real Web Socket server.