Use ws WebSocket when options are provided
#2968
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.7-dev #2968 +/- ##
=============================================
+ Coverage 76.14% 76.37% +0.22%
- Complexity 13152 13232 +80
=============================================
Files 1084 1061 -23
Lines 65160 61574 -3586
Branches 7285 7341 +56
=============================================
- Hits 49616 47025 -2591
+ Misses 12839 12030 -809
+ Partials 2705 2519 -186 ☔ View full report in Codecov by Sentry. |
|
The only thing that might be a problem is that there may be a small set of users who will now use Thanks for helping to make gremlin-javascript more compatible with later versions of Node. VOTE +1 |
|
@kenhuuu is this good to merge? 😁 |
|
Yes, it passed lazy consensus since you got my vote. However, I don't know JavaScript or gremlin-javascript that well so I'd like to keep it open a little while longer just to give an opportunity for another contributor to review it. If there is no other review done by this Friday the 24th, then I'll merge this and the other PR you opened. |
| return connection.open(); | ||
| }); | ||
| Promise.allSettled(allOptionTests).then(function () { | ||
| if (globalWebsocketCalls !== domUnsupportedOptions.length) { |
There was a problem hiding this comment.
@KyleBoyer Where does domUnsupportedOptions come from?
And in this test, wouldn't globalWebsocketCalls === 0?
There was a problem hiding this comment.
Ahh, that should be / was renamed towsSpecificOptions. I'll push that change shortly.
Correct, in this test the globalWebsocketCalls should be 0.
There was a problem hiding this comment.
@kmcginnes Changes pushed!
Going to keep this open for a little bit longer since there was a recent comment. |
|
@kenhuuu sounds good, I'm just hoping to get it in before the next 3.7.X release! |
| }); | ||
| return connection.open(); | ||
| }); | ||
| Promise.allSettled(allOptionTests).then(function () { |
There was a problem hiding this comment.
If I'm not mistaken, this promise should be returned from the test function so that Jest waits on it properly.
There was a problem hiding this comment.
Good catch, pushed the fix for this! Thank you :)
| Promise.allSettled(allOptionTests).then(function () { | ||
| if (globalWebsocketCalls !== 0) { | ||
| assert.fail('global WebSocket should be used when no ws specific options are provided'); | ||
| } |
There was a problem hiding this comment.
This may be subjective, but could this if statement be an assertion instead?
assert.equal(globalWebsocketCalls, 0, 'global WebSocket should be used when no ws specific options are provided');There was a problem hiding this comment.
Pushed the change for this!
| .finally(function () { | ||
| if (globalWebsocketCalls <= 0) { | ||
| assert.fail('global WebSocket should be used when no ws specific options are provided'); | ||
| } |
There was a problem hiding this comment.
Same question for this if statement.
There was a problem hiding this comment.
Pushed the change for this!
The changes requested have been pushed! Thank you for reviewing :) |
|
LGTM! +1 |
|
Thanks again @KyleBoyer for your contributions. VOTE +1 |
The current 3.7-dev implementation does not pass
headersto the WebSocket when utilizing theglobalThis.WebSocket. This causes issues when initially upgrading to this version of gremlin (or upgrading to Node 22 when the global WebSocket class was enabled by default). This PR ensures that we use thewsWebSocket implementation ifwsspecific options are passed along.