Fix WebSocket API inconsistency in Gremlin connection #3204
Conversation
Fix WebSocket API inconsistency causing "this._ws.on is not a function" error The Gremlin JavaScript driver was mixing browser WebSocket API (.addEventListener) with Node.js WebSocket API (.on) in the same connection logic. This caused runtime errors when globalThis.WebSocket was detected but the browser WebSocket implementation doesn't support the .on() method used for 'unexpected-response' events. Changes: - Conditionally attach 'unexpected-response' listener only for Node.js WebSocket (ws package) - Preserve existing useGlobalWebSocket logic for legitimate browser/polyfill scenarios - Update cleanup method to safely remove listeners based on WebSocket type This maintains backward compatibility while fixing the cross-platform API incompatibility that prevented connections in environments with WebSocket polyfills or global WebSocket objects.
|
The test that fails is due to existing code that I have kept. Do you want to keep it or should I remove it? |
| this._ws?.off('unexpected-response', this.#handleUnexpectedResponse); | ||
| // Only remove unexpected-response listener for Node.js WebSocket (ws package) | ||
| // Browser WebSocket does not have this event and .off() method | ||
| if (this._ws && 'off' in this._ws) { |
There was a problem hiding this comment.
I am wondering if the same check as above !useGlobalWebSocket would also work here or is there a particular reason to use different if conditions for .on vs .off?
Which test is failing exactly? I can't tell from the logs. It seems like the change is backwards compatible so I am wondering what is failing. |
lib/driver/connection.ts(477,7): error TS2578: Unused '@ts-expect-error' directive. |
|
Hi @xhuberty can you please change the PR to rebase and target 3.7-dev branch? |
Yes that should be removed as @ts-expect-error is unnecessary after your changes |
|
I will update the PR next week. |
Fix WebSocket API inconsistency causing "this._ws.on is not a function" error
The Gremlin JavaScript driver was mixing browser WebSocket API (.addEventListener)
with Node.js WebSocket API (.on) in the same connection logic. This caused runtime
errors when globalThis.WebSocket was detected but the browser WebSocket implementation
doesn't support the .on() method used for 'unexpected-response' events.
Changes:
This maintains backward compatibility while fixing the cross-platform API incompatibility
that prevented connections in environments with WebSocket polyfills or global WebSocket objects.
This is related to https://issues.apache.org/jira/browse/TINKERPOP-3160