-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
refactor: transportMode and client URL options #3260
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
refactor: transportMode and client URL options #3260
Conversation
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.
Lgtm. Nice comments on new code as well👏🏽
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 great! Let's fix CI 🚀
I found better way without big changes |
Hard... Now we have more clearly options:
It's all pretty flexible but may be slightly misleading. Maybe somebody have ideas how to improve this (renaming/clear description/etc). |
/cc @snitin315 maybe you have ideas too |
The change looks good to me. I think we can roll it in next beta release for more feedback and see if developers files any complaints or finds it confusing. |
Maybe we don't need |
@snitin315 Found cases, I think we need better name, maybe |
I'll try to figure it out by tomorrow, I will be glad for any help, we need to finish the release as soon as possible... |
Maybe In the meantime, I will fix the tests here. |
b8ba623
to
b1bb5c1
Compare
e08f597
to
d6f9a30
Compare
Yep, let's solve separately, also we need refactor our logic to inject and remove |
d6f9a30
to
b4e38a0
Compare
…port-mode-options' into refactor-client-and-server-transport-mode-options # Conflicts: # test/server/__snapshots__/transportMode-option.test.js.snap.webpack5
b4e38a0
to
4d5307b
Compare
I will finish PR, we need rewrite some tests + add more them, give me 30-40 minutes |
@@ -9,12 +9,9 @@ module.exports = { | |||
entry: resolve(__dirname, './foo.js'), | |||
devServer: { | |||
client: { | |||
transport: 'sockjs', |
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 Again, here you do the same problem as do other developers, client.transport
for only client, it is not create sockjs server, we need search better name for this, you should not set sockjs
/ws
server using this options, it is only if you want create custom web socket client
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 see we need configure webSocketServer
here.
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.
Yep, I am working on it, most of tests are good, anyway we need refactor many tests, I will create roadmap for rc for tests and how we need rewrite them
4d5307b
to
7405bb6
Compare
Codecov Report
@@ Coverage Diff @@
## master #3260 +/- ##
==========================================
- Coverage 95.56% 95.54% -0.02%
==========================================
Files 34 34
Lines 1241 1258 +17
Branches 354 356 +2
==========================================
+ Hits 1186 1202 +16
- Misses 51 52 +1
Partials 4 4
Continue to review full report at Codecov.
|
@snitin315 @anshumanv let's finish this firstly before merge other, each rebase is harder and harder |
…port-mode-options' into refactor-client-and-server-transport-mode-options
Yep lets focus to get this in first 👍 |
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.
👍
For Bugs and Features; did you add new tests?
WIP
Motivation / Use-Case
I want to split #3181 on multiple PRs/commits
{ options.transportMode: { server: { type: 'ws', options: { path: '/ws' } } } }
and{ options.transportMode: { client: { type: 'ws', options: { path: '/ws' } } } }
, now we can pass options to websocket server, also should fix devServer.sockPath #2535, so you can passclient.host
/client.port
tooptions.transportMode.client.options
transportMode
andclient
options we need discussion here how better name this option, we haveserver
(i.e. all top level options likehost
,port
and etc),websocket
server (i.e.ws
/sockjs
packages and options, for exampleoptions.transportMode.server.options.path
) andclient
(i.e. websocket API/sockjs client and options for client likeclient.overlay
/client.logging
/etc), and here we have problems our options mixed betweenoptions.transportMode.client
and top levelclient
(step 2 partial fix it, but I think we can simplify it and do it better)/cc @webpack/cli-team
Breaking Changes
Yes
Additional Info