Add WebSocket handler for Browser and Node #9191
Conversation
|
There was a problem hiding this comment.
See the comments for more but I'm leaning towards not offering this for Node given the complications and not being sure there's a big demand for it anyway. I feel really bad about that given the effort you've put into the Node implementation and the clever dead code trick but it might come back.
Size Report 1Affected ProductsNo changes between base commit (a4897a6) and merge commit (a06a3a1).Test Logs |
Size Analysis Report 1Affected ProductsNo changes between base commit (a4897a6) and merge commit (a06a3a1).Test Logs |
| * limitations under the License. | ||
| */ | ||
|
|
||
| import { NodeWebSocketHandler } from './node/websocket'; |
There was a problem hiding this comment.
Does this work? The rollup alias just replaces the node in the import path, it doesn't replace the name of the imported symbol, right? Isn't this supposed to be just WebSocketHandlerImpl or something and the exported symbol from node/websocket.ts and browser/websocket.ts both have the same name?
There was a problem hiding this comment.
This file platform/websocket.ts is only used during tests ran with ts-node, where the rollup alias isn't applied.
In the code that is bundled, we should not import BrowserWebSocketHandler or NodeWebSocketHandler directly. Instead, we should use the factory method createWebSocketHandler.
We could make the class names identical WebSocketHandlerImpl, but I prefer them being more explicit for readability reasons if there's no technical requirement for them to have the same name.
There was a problem hiding this comment.
Oh I see, you just don't have an example of a prod code consumer calling createWebSocketHandler in this PR.
Adds a WebSocket handler with implementations in Browser and Node environments. This will be used for BiDi.
Changes:
WebSocketHandler:BrowserWebSocketHandler) and Node (NodeWebSocketHandler).NodeWebSocketHandleruses the built-in globalWebSocketavailable in Node >= 22.wsas a dev dependency for types and the mock test server (Node 22 doesn't support web socket servers).BrowserWebSocketHandlerusing a mockedwindow.WebSocket.NodeWebSocketHandlerusing a localMockWebSocketServer.test:nodegenerateAliasConfigto the set of rollup helpers, and uses it for platform import aliasing.