-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: add webSocketURL option #3309
Conversation
no need review drafts, let me finish |
675e4b0
to
3507a4d
Compare
Codecov Report
@@ Coverage Diff @@
## master #3309 +/- ##
==========================================
- Coverage 95.70% 95.63% -0.07%
==========================================
Files 34 33 -1
Lines 1280 1283 +3
Branches 369 369
==========================================
+ Hits 1225 1227 +2
- Misses 51 52 +1
Partials 4 4
Continue to review full report at Codecov.
|
@alexander-akait @snitin315 CI should be green now 😎 |
@anshumanv Do you need help? I can finish, it is blocker for the next |
@alexander-akait should be green now, feel free to make any changes or lmk I can do it, thanks for help 👍 |
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.
Need to remove public
option.
Let's finish 👍 WIP |
Finished... We still have some edge cases and places where we can simplify code, but I will do it in the next PRs |
Also need to remove |
Oh, yes, forgot about it |
I will remove this option in this PR and let's add all options here #3325 |
All our tests are complete horror, we need to rewrite them, I spent half a day to finish them |
@snitin315 @anshumanv Can you review? |
webSocketURL: { | ||
path: '/custom/path/', | ||
}, |
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.
We should add one case with webSocketURL: 'ws://myhost.com:8080/path'
, and check it normalizes host, port and path correctly from the string.
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.
Will the webSocketURL string allow me to provide wss:// and actually have the browser request using wss://, or will the normalization turn it back into ws:// if https is false? I'm running a reverse https -> http proxy in front, so the dev server should be https:false, but currently doing so, the websocket logic attempts to use ws:// even though the address bar is an https:// url. Just checking to see if this string webSocketURL option will indeed allow me to now force wss:// to be used for the websocket requests.
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.
@bompus you can use ws
and wss
, and even http
/https
(we normalize them to ws
/wss
for backward compatibility). But you should use webSocketURL
only when you proxy dev server, if you set https: true
we will use wss
by default
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.
@snitin315 we have this test (in test/e2e/web-socket-server-and-url.test.js
):
client: {
webSocketURL: `ws://myhost.test:${port2}/foo/test/bar/`,
},
`
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.
All good then 👍🏻
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 assume one would need to use auto://0.0.0.0:0/wspath/ if they wanted a specific path?
Yes, maybe make sense to add auto
to path too
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.
Great. This should solve my use case 👍
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.
Put in todo, will solve tomorrow
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.
@alexander-akait did you get a chance to start on this yet, or still pending? Just want to make sure it doesn't get buried.
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.
in my todo on tomorrow, don't worry 👍
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.
looks good 👍
For Bugs and Features; did you add new tests?
migrated existing
Motivation / Use-Case
Move out client.host/port/path to a new option
webSocketURL
Breaking Changes
Yes
Additional Info