Skip to content

Do not override sockProtocol from config#835

Merged
pmmmwh merged 2 commits intopmmmwh:mainfrom
darkexone:main
Apr 25, 2024
Merged

Do not override sockProtocol from config#835
pmmmwh merged 2 commits intopmmmwh:mainfrom
darkexone:main

Conversation

@darkexone
Copy link
Copy Markdown
Contributor

@darkexone darkexone commented Apr 16, 2024

Hey, I came across a case where sockProtocol is incorrectly overwritten. My application does not use https, but is served through a proxy that uses https. I have https disabled in the webpack configuration, but the protocol read from the window is https. I tried passing the sockProtocol parameter, but found that wss is used all the time, resulting in itegration with overlay not working. I believe that the config passed by the developer (a resourceQuery) should take priority over a failsafe configuration.

A better idea would be to make a mechanism for reading the webSocketURL from the Webpack configuration (as suggested in #742 and #728), because this fix will help only for part of the problems with connecting to the wrong sockets. Until this mechanism is implemented, we should prefer the plugin configuration over the values read from the path.

@codesandbox
Copy link
Copy Markdown

codesandbox bot commented Apr 16, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@pmmmwh
Copy link
Copy Markdown
Owner

pmmmwh commented Apr 25, 2024

A better idea would be to make a mechanism for reading the webSocketURL from the Webpack configuration (as suggested in #742 and #728), because this fix will help only for part of the problems with connecting to the wrong sockets. Until this mechanism is implemented, we should prefer the plugin configuration over the values read from the path.

With the later versions of WDS v4 and WDS v5 we can actually re-use the "created" server so we no-longer need to parse any of their config. It'll probably come in the next version though.

@pmmmwh pmmmwh merged commit 88e1441 into pmmmwh:main Apr 25, 2024
@pmmmwh
Copy link
Copy Markdown
Owner

pmmmwh commented Apr 25, 2024

Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants