allow explicitly setting the protocol from the public option #1117

robertaistleitner · 2017-09-26T14:07:56Z

What kind of change does this PR introduce?
public options now allows forcing the protocol for the socket connection.

Did you add or update the examples/?
No.

Summary
There are issues if using entrypoints in iframes, more infos in issue #1116.

Does this PR introduce a breaking change?
No.

jsf-clabot · 2017-09-26T14:07:58Z

All committers have signed the CLA.

shellscape

looks like you have some linting errors to clean up before we can consider merging this PR. please run tests before pushing so you can catch these kinds of errors before it gets to CI.

please also add a new example created (preferably called cli-public-protocol similar to cli-public which shows folks how to use this change, and will also serve as a functional test.

shellscape · 2017-09-26T14:37:25Z

lib/util/createDomain.js

-    port
-  });
+  if (options.public) {
+    return options.public.indexOf('://') > 0 ? `${options.public}` : `${protocol}://${options.public}`;


this is not a good way to check a url for a protocol. please use url.parse instead, and examine the result for a protocol.

+ found one other glitch that auto-reload doesn't work in iframes since it tries to reload the iframe itself (which has src `about:blank`) and therefore cannot be reloaded properly. I'm now checking for protocol `about:` and traverse upwards until a reloadable window can be found. Feedback on this fix is welcome!

codecov · 2017-09-27T08:15:26Z

Codecov Report

Merging #1117 into master will increase coverage by 5.22%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1117      +/-   ##
=========================================
+ Coverage   71.58%   76.8%   +5.22%     
=========================================
  Files           5       5              
  Lines         468     470       +2     
  Branches      150     151       +1     
=========================================
+ Hits          335     361      +26     
+ Misses        133     109      -24

Impacted Files	Coverage Δ
lib/util/createDomain.js	`100% <100%> (+62.5%)`	⬆️
lib/Server.js	`83.73% <0%> (+5.72%)`	⬆️

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 ee7231b...e170f18. Read the comment docs.

+ found a bug due to tests: `url.parse` doesn't properly parse a provided `public` option since it counts everything before the port colon as the protocol which is just wrong. I replaced it with a protocol regex.

robertaistleitner · 2017-09-27T09:48:17Z

Hey guys, just checked why the tests failed (they work on my machine), and it seems the timeout with 2 seconds is a little too low, is there a possibility to increase it? The test case which fails is generating a ssl certificate (option https is set to true for server).

shellscape · 2017-09-27T13:41:38Z

client/index.js

-    self.location.reload();
+    let rootWindow = self;
+    // use parent window for reload (in case we're in an iframe with no valid src)
+    do {


this is almost never a good idea in the browser - on the off chance that the while condition is not met, this will lock the browser forcing a close. please use setInterval in conjunction with clearInterval. the log message should also be moved to just before the actual call to reload

I don't really get what you mean with using setInterval and clearInterval here and how this will improve the code.

+1 for moving the log down

shellscape

the do/while loop needs to be replaced by setInterval and clearInterval. looks like the comment on url.parse was missed and the check for protocol was replaced by a regex.

I applaud the extensive example you created, but unfortunately it's far too complex. all that's needed is the basic cli-public example cloned and using a full url with protocol. if this is confusing you're welcome to leave that out and I'll add it after the merge.

shellscape · 2017-09-27T13:44:53Z

lib/util/createDomain.js

@@ -10,8 +10,12 @@ module.exports = function createDomain(options, listeningApp) {
  const port = options.socket ? 0 : appPort;
  const hostname = options.useLocalIp ? internalIp.v4() : options.host;

+  // use explicitly defined public url (prefix with protocol if not explicitly given)
+  if (options.public) {
+    return /^[a-zA-Z]+:\/\//.test(options.public) ? `${options.public}` : `${protocol}://${options.public}`;


sorry, but this is not what was requested in previous comments

this is not a good way to check a url for a protocol. please use url.parse instead, and examine the result for a protocol.

I wrote in the commit comment that url.parse is not appropriate here.. I did it with url.parse before but the new tests I provided showed it's not working as it should..

found a bug due to tests: url.parse doesn't properly parse a provided public option since it counts everything before the port colon as the protocol which is just wrong. I replaced it with a protocol regex.

Sorry, but I looked up url.parse documentation again, as well as searching for issues with what I'm experiencing, and there's no easy solution possible by using url.parse.

Why? The public parameter can be a non-valid url like "localhost:8080" which is parsed to somethin like this:

> url.parse("localhost:8080") Url { protocol: 'localhost:', slashes: null, auth: null, host: '8080', port: null, hostname: '8080', hash: null, search: null, query: null, pathname: null, path: null, href: 'localhost:8080' }

As you can see, the protocol is incorrect and I would need to do more processing to find out what's going on. I think the solution using regex is the better option here.

Also have a look at nodejs/node#5755, where they're discussing the same issue on parsing urls.

great documentation of the why. that'll certainly be beneficial in the future

robertaistleitner · 2017-09-28T07:29:29Z

Regarding the too complex example: there's no other way to show why this is even needed (reverse proxy running on https + iframes + dev-server) so I don't get how the simple scenario should show how and why it works.

shellscape · 2017-09-28T13:45:32Z

@robertaistleitner thanks for all your attention and work on this so far. it seems we're at odds with what I'd like to see in this PR and how you'd like to see it. I understand that you had issues with url.parse but I don't feel they're blocking as you say, and I feel strongly that a regex is not appropriate in this case. the differences between a do/while loop and set/clearInterval in the browser are significant, and do/while's ability to block/lock the browser in an errant condition is very widely known. examples also exist to demonstrate options, but not to demonstrate all-encompassing solutions, and shouldn't involve external tools that require additional user action; if you'd prefer, you can remove your example and I'll add one after the fact.

in the PR's current form I just can't merge it, citing my comments. apologies for being hard on this one but I can't budge on that I'm afraid. if you'd like to abandon, we'll schedule these changes for the v3 and cite your issue for credit. if you'd like to move forward with the requested changes, we'd be happy to continue the review.

robertaistleitner · 2017-10-02T07:34:34Z

Sorry for not writing back sooner, I definitely want to get this merged and I'll propose solutions for the requested stuff as soon as possible..

* remove docker stuff from examples (minimize example) * use setInterval and clearInterval instead of do while loop when getting root window for reload

shellscape · 2017-10-08T23:53:00Z

Going to merge this one in now, thanks for all the work on it and apologies it took so long. Please note that it may take a while to get to release. The version 3 effort is in full swing and there's a pending refactor of the client code that I'll need to manually merge your changes into.

robertaistleitner · 2017-10-09T11:53:16Z

Thanks, I'm happy it got merged!

allow explicitly setting the protocol from the public option

eecad00

shellscape mentioned this pull request Sep 26, 2017

public option doesn't allow explicitly to set protocol #1116

Closed

2 tasks

shellscape suggested changes Sep 26, 2017

View reviewed changes

robertaistleitner added 2 commits September 27, 2017 09:54

fix linting issues

972825e

provide tests for createDomain helper function

650182f

+ found a bug due to tests: `url.parse` doesn't properly parse a provided `public` option since it counts everything before the port colon as the protocol which is just wrong. I replaced it with a protocol regex.

robertaistleitner and others added 2 commits September 27, 2017 11:56

increase timeout for https test which exceeds default 2000ms

21c54c6

Merge branch 'master' into master

b1a4eb8

shellscape reviewed Sep 27, 2017

View reviewed changes

shellscape suggested changes Sep 27, 2017

View reviewed changes

robertaistleitner added 2 commits September 28, 2017 09:30

Merge branch 'master' into master

ef0e57b

move info log to the place where the reload happens

9e6b272

robertaistleitner and others added 3 commits October 6, 2017 13:03

apply feedback

cfdb070

* remove docker stuff from examples (minimize example) * use setInterval and clearInterval instead of do while loop when getting root window for reload

increase test timeout for https to 60 seconds

f481d0f

Merge branch 'master' into master

e170f18

shellscape approved these changes Oct 8, 2017

View reviewed changes

shellscape merged commit c490b24 into webpack:master Oct 8, 2017

renovate bot mentioned this pull request Mar 21, 2018

Update webpack-dev-server to 3.1.4-abandoned sbardian/DebtTrackerAPI#22

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow explicitly setting the protocol from the public option #1117

allow explicitly setting the protocol from the public option #1117

robertaistleitner commented Sep 26, 2017

jsf-clabot commented Sep 26, 2017 •

edited

shellscape left a comment

shellscape Sep 26, 2017

codecov bot commented Sep 27, 2017 •

edited

robertaistleitner commented Sep 27, 2017

shellscape Sep 27, 2017 •

edited

robertaistleitner Sep 28, 2017

shellscape left a comment

shellscape Sep 27, 2017

robertaistleitner Sep 28, 2017

robertaistleitner Oct 6, 2017

shellscape Oct 8, 2017

robertaistleitner commented Sep 28, 2017

shellscape commented Sep 28, 2017

robertaistleitner commented Oct 2, 2017

shellscape commented Oct 8, 2017

robertaistleitner commented Oct 9, 2017

allow explicitly setting the protocol from the public option #1117

allow explicitly setting the protocol from the public option #1117

Conversation

robertaistleitner commented Sep 26, 2017

jsf-clabot commented Sep 26, 2017 • edited

shellscape left a comment

Choose a reason for hiding this comment

shellscape Sep 26, 2017

Choose a reason for hiding this comment

codecov bot commented Sep 27, 2017 • edited

Codecov Report

robertaistleitner commented Sep 27, 2017

shellscape Sep 27, 2017 • edited

Choose a reason for hiding this comment

robertaistleitner Sep 28, 2017

Choose a reason for hiding this comment

shellscape left a comment

Choose a reason for hiding this comment

shellscape Sep 27, 2017

Choose a reason for hiding this comment

robertaistleitner Sep 28, 2017

Choose a reason for hiding this comment

robertaistleitner Oct 6, 2017

Choose a reason for hiding this comment

shellscape Oct 8, 2017

Choose a reason for hiding this comment

robertaistleitner commented Sep 28, 2017

shellscape commented Sep 28, 2017

robertaistleitner commented Oct 2, 2017

shellscape commented Oct 8, 2017

robertaistleitner commented Oct 9, 2017

jsf-clabot commented Sep 26, 2017 •

edited

codecov bot commented Sep 27, 2017 •

edited

shellscape Sep 27, 2017 •

edited