fix:Uncaught SyntaxError: The URL 'http:/sockjs-node' is invalid

[EslamHiko]

• [ x] This is a bugfix
• This is a feature
• This is a code refactor
• This is a test update
• This is a docs update
• This is a metadata update

For Bugs and Features; did you add new tests?

Tests already exist

Motivation / Use-Case

to solve this issue : #1743

Breaking Changes

I just replaced socketUrl with '/sockjs-node' and removed unused code.

Additional Info

I'm new to open source contributing if there is anything wrong about my pull request or any advice please tell me I'll really appreciate it.

[jsf-clabot]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #1749 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1749   +/-   ##
=======================================
  Coverage   86.85%   86.85%           
=======================================
  Files           9        9           
  Lines         578      578           
  Branches      170      170           
=======================================
  Hits          502      502           
  Misses         63       63           
  Partials       13       13

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f899ff...34160ec. Read the comment docs.

[knagaitsev]

Oops, it looks like we were working on the same fix at the same time #1750. The issue with your fix is that it disregards the sockPath option, which allows users to change that path from /sockjs-node to something else https://webpack.js.org/configuration/dev-server/#devserversockpath. Also, you should not ignore if the user requests a specific hostname, nor should you ignore if a user wants https. That is what the block of code is doing that you removed. Be sure to consider other option cases, even if it works in one particular case!

This does reveal that more tests are needed, since your changes passed CI. Maybe something more is needed here https://github.com/webpack/webpack-dev-server/blob/master/test/Socket.test.js or here https://github.com/webpack/webpack-dev-server/blob/master/test/Client.test.js to confirm that the client actually connects to the sockPath that the server provides.

[EslamHiko]

@Loonride Thank you, I understand now what was wrong....
about the testing part, I noticed here: https://github.com/webpack/webpack-dev-server/blob/master/test/Socket.test.js
at line 27 req.get('/sockjs-node').expect(200, done); should it be req.get(server.sockPath).expect(200, done); or should i add something else ?

[knagaitsev]

That test works how it should, since it is confirming that the default behavior of the server is to use /sockjs-node when the the sockPath option is not provided. In the tests, hard coding things like that is fine, because the goal of the tests is to confirm that the server is following some strict behavior, and if it is not, the test should fail.

I added a test in my PR which confirms that the client connects to the right sockPath: https://github.com/webpack/webpack-dev-server/pull/1750/files#diff-b2d4a9632f66a16163b3edfdf960185e. The test runs a browser, then waits for the simulated client to make a request to the correct hostname and sockPath.

It may be redundant at this point for you to fix your PR, but I recommend you look at the test above and make sure you understand why what you wrote would not pass that test. If you have any other questions, I would be happy to answer them!

[EslamHiko]

@Loonride I got the idea about what the test is testing that was really helpful for me, I'm going to learn more about tests to get a full understanding of exactly what's going on in the test.
I really appreciate your comments, Thank you.

[alexander-akait]

/cc @EslamHiko so we can close PR or you continue work in future?

[EslamHiko]

@evilebottnawi Yes sure no problem at all 😄