fix(client): don't override protocol for socket connection to 127.0.0.1

[nougad]

Chrome and Firefox accept http:///ws:// mixed-content connection to
127.0.0.1 even when the actual website is loaded via https://.

Fixes #2302

• 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?

I'm not sure if the current tests actually contain correct assertions.

Motivation / Use-Case

described in #2302

Breaking Changes

nope

Additional Info

[jsf-clabot]

All committers have signed the CLA.

[hiroppy]

Need to add tests.

[codecov]

Codecov Report

Merging #2303 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2303      +/-   ##
==========================================
+ Coverage   93.83%   93.84%   +<.01%     
==========================================
  Files          34       34              
  Lines        1282     1283       +1     
  Branches      365      366       +1     
==========================================
+ Hits         1203     1204       +1     
  Misses         78       78              
  Partials        1        1

Impacted Files	Coverage Δ
client-src/default/utils/createSocketUrl.js	96.96% <100%> (+0.09%)	⬆️

---

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 f501e83...6cc5853. Read the comment docs.

[nougad]

Thank you @hiroppy for your fast comment. Looking at the current tests it seams like the serialization of the URL object generated wrong snapshots:

https://github.com/webpack/webpack-dev-server/blob/master/test/client/utils/__snapshots__/createSocketUrl.test.js.snap#L13

That makes adding new tests hard since the assertions are arbitrary. Is my assumption correct? Is there a ticket for replacing the snapshot with actual tests?

[hiroppy]

I think this line is incorrect.

webpack-dev-server/client-src/default/utils/createSocketUrl.js

Line 14 in 00903f6

urlParts = url.parse(resourceQuery.substr(1));

Is there a ticket for replacing the snapshot with actual tests?

Currently no. I'll check this. thanks!

[nougad]

Awesome! let me know when you found the bug. I'm happy to add tests for this PR 😄

[odbayar]

Will it work for "localhost" too?

[nougad]

@odbayar AFAIK it only works with 127.0.0.1.

@hiroppy That line is actually correct. The method expects only a query like ?http://0.0.0.0&asdf=32 as parameter but the tests provide a full URL. If I remove the substr other tests fail.

I updated the tests to pass only the querystring as parameter but that lines are fishy as well:

webpack-dev-server/client-src/default/utils/createSocketUrl.js

Lines 19 to 22 in 00903f6

	if (scriptHost) {
	// eslint-disable-next-line no-useless-escape
	scriptHost = scriptHost.replace(/\/[^\/]+$/, '');
	}

which leads to wrong tests like that one:

https://github.com/webpack/webpack-dev-server/blob/master/test/client/utils/__snapshots__/createSocketUrl.test.js.snap#L23-L25

I removed the block since it looks unused and I could not make any sense out of it.

Lastly I cleaned some unrelated unit tests to use the correct assertions and refactored the createSocketUrl file to make it easier testable. I made everything a separate commit. Let me know if you want separate PRs instead.

[nougad]

@hiroppy could you please have a second look if the improvements for unit testing make sense? Thanks

[alexander-akait]

Looks like big breaking change, why a lot of lines was rewritten? Please avoid unnecessary changes in this PR

[nougad]

@evilebottnawi I understand this change was growing a bit more than expected over time. Please have a look at the individual commits:

• 75e620a - thats the actual change. But as @hiroppy pointed out above, the unit tests for that file are incorrect
• 0df1f0c fixes then the unit tests for the file
• d1a86d7 is a general improvement of unit test assertions and I'm happy to discard/move into a different PR
• 8b0ef4a is the actual big diff because it introduces some refactoring into the file to make it easier to test. Again, I'm happy of not adding those additional tests/refactoring or move it into a second PR to unblock the actual change.

Please let me know which change you want in a separate PR or if it's ok to review it as part of this since everything is kind of related.

[alexander-akait]

@nougad let's do refactor and fix tests in other PRs for better git history and good changelog and then we will return to this PR

[nougad]

I created two separate PRs with fixing tests and improving assertions:

• fix(client): fix tests for createSocketUrl #2323 - fix(client): fix tests for createSocketUrl
• fix(test): replace include.toBeTruthy with toContain #2324 - fix(test): replace include.toBeTruthy with toContain

I will update this PR here one the test fix is merged since it's a prerequisite to have working unit tests.

[alexander-akait]

/cc @nougad need rebase 👍

[nougad]

@evilebottnawi rebased and updated to only include the one relevant commit.

Once merged I will create another PR with the refactoring on top.

[alexander-akait]

/cc @hiroppy @Loonride

[nougad]

Is there anything left for this PR? I promise even more extensive testing with the next PR once this one is merged :) nougad@acd46c8

[hiroppy]

thanks

[BinarySpike]

Could this be expanded to localhost as well?