Skip to content
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

Allow changing socket.io-path via query string #240

Merged
merged 1 commit into from
Sep 11, 2015
Merged

Allow changing socket.io-path via query string #240

merged 1 commit into from
Sep 11, 2015

Conversation

syranide
Copy link
Contributor

Related #46, #220, (kind of) #239

This PR allows you to use 'webpack-dev-server/client?http://mydomain/mysocket.io/' (domain is optional, just as path is). I.e. being able to change the socket.io path too and not just the domain. If there is no path you get the current behavior. url.parse parses http://example.com as having path / (so we must consider it no path), but that's not really a problem and preferable to avoid breaking existing configs.

If anyone wants to try it:
"webpack-dev-server": "git://github.com/syranide/webpack-dev-server#socketiopath_npm"

cc @sokra

@kentor
Copy link

kentor commented Sep 7, 2015

Does this allow changing the domain? It's hard to test in VM where I use 10.0.2.2 instead of localhost to access the dev server asset, but the script still tries to access localhost for the socket.

@syranide
Copy link
Contributor Author

syranide commented Sep 7, 2015

@kentor That already works, this allows changing the path too.

sokra added a commit that referenced this pull request Sep 11, 2015
Allow changing socket.io-path via query string
@sokra sokra merged commit bd425b8 into webpack:master Sep 11, 2015
@sokra
Copy link
Member

sokra commented Sep 11, 2015

Thanks

@syranide syranide deleted the socketiopath branch September 11, 2015 07:13
@SimenB
Copy link
Contributor

SimenB commented Sep 17, 2015

This destroys HMR for us. Using client/index.js as it was before this PR works. 1.10.1 works, 1.11.0 does not.

First it says [HMR] Waiting for update signal from WDS... then just the screenshot.

Our entry is webpack-dev-server/client?http://:::3030 This was the only way to make it work cross-OS for us (at least at the time, might have changed)

image

@syranide
Copy link
Contributor Author

Hmm that's an interesting edge-case, you want to set the port but not the host?

@SimenB
Copy link
Contributor

SimenB commented Sep 17, 2015

That's correct. We always know the port as that's the port of WebpackDevServer. But the host IP changes. It's basically localhost, but setting either localhost or 0.0.0.0 makes it not available on the LAN. We want to access the running server from other machines than just the one running the dev-server

@syranide
Copy link
Contributor Author

It seems pretty clear to me that http://:::3030 was a hack and should not be valid. It also seems to me that there's a bug in url.parse, when parsed and formatted it ends up as http://:3030/::, that seems nonsensical (but perhaps that simply an acceptable artifact due to the malformed url?).

Anyway, I imagine the correct approach here would be to special-case 0.0.0.0 and have it too be substituted with location.host.

cc @sokra

@SimenB
Copy link
Contributor

SimenB commented Sep 17, 2015

http://:::3030 is perfectly valid IPv6. https://en.wikipedia.org/wiki/IPv6_address#Presentation

We should be able to use 0.0.0.0, though...

@syranide
Copy link
Contributor Author

@SimenB True, in that case then it's really a bug in url.parse that should be fixed in node and 0.0.0.0 won't be necessary. Also... I got lost in my thoughts a bit, http://:3030 actually parses and formats correctly, however it ends up going wrong somewhere eventually and ends up as http://http, so it seems the problem really is elsewhere. So that needs to be investigated, but it seems to me right now that this is not strictly a bug in my PR (it just closed a previous loophole and is victim of a bug in url).

PS: If :: was handled correctly before then it should work to outright substitute it with 0.0.0.0 I would think?

@syranide
Copy link
Contributor Author

Hmm, it seems 0.0.0.0 already happened...
eb50101 https://github.com/webpack/webpack-dev-server/blob/master/client/index.js#L15

EDIT: It still makes sense to look into what's going on here, especially if the parsing is actually a bug in nodejs.

@SimenB
Copy link
Contributor

SimenB commented Sep 17, 2015

Using 0.0.0.0 works now. No idea why it didn't before... It's been that way for 6 months, I don't really remember why we went with ::.

Edit: I'll have to test on a WIndows machine tomorrow, will report back. 0.0.0.0 works on my Linux at least

@kentor
Copy link

kentor commented Sep 17, 2015

@SimenB where do you specify that? I'm guessing that would solve my issue #262

@SimenB
Copy link
Contributor

SimenB commented Sep 17, 2015

Specify what? The entry?

  webpackConfig.entry = [
    'webpack-dev-server/client?http://0.0.0.0:3030',
    'webpack/hot/only-dev-server',
    path.join(__dirname, 'js/app.js')
  ];

@syranide
Copy link
Contributor Author

@SimenB 0.0.0.0 was added at the same time as my PR it seems, so just this last version.

@SimenB
Copy link
Contributor

SimenB commented Sep 17, 2015

Ah, ok. Makes sense 😄

@syranide
Copy link
Contributor Author

@SimenB Also, FYI nodejs/node#2929 (comment)

@SimenB
Copy link
Contributor

SimenB commented Sep 17, 2015

Hangs forever using 'webpack-dev-server/client?http://[::]:3030' Doesn't throw the error though

image

@kentor
Copy link

kentor commented Sep 17, 2015

@SimenB That works for connecting to the socket. But what do you specify as the output.publicPath?

@SimenB
Copy link
Contributor

SimenB commented Sep 18, 2015

@kentor output.publicPath = '/'

@SimenB
Copy link
Contributor

SimenB commented Sep 18, 2015

Doesn't work on windows, so for now we're stuck on 1.10.1...

image

@syranide
Copy link
Contributor Author

@SimenB The final URL is really funky, including encoded chars (which seems to indicate an URL being encoded as a hostname inside an URL) ... what have you written in your config? I'm assuming the URL you are visiting is http://localhost:3030?

https://github.com/webpack/webpack-dev-server/blob/master/client/index.js#L13

@SimenB
Copy link
Contributor

SimenB commented Sep 18, 2015

Entry is still 'webpack-dev-server/client?http://0.0.0.0:3030', and yes, localhost

@syranide
Copy link
Contributor Author

@SimenB Hmm, I don't see how that ends up being translated to what you printscreened above... could you perhaps put some console.logs in https://github.com/webpack/webpack-dev-server/blob/master/client/index.js#L13 and see where it's going wrong?

// on line 10
console.log(urlParts);
console.log(window.location.hostname);
console.log(
    url.format({
        protocol: urlParts.protocol,
        auth: urlParts.auth,
        hostname: (urlParts.hostname === '0.0.0.0') ? window.location.hostname : urlParts.hostname,
        port: urlParts.port
    })
);

@syranide
Copy link
Contributor Author

Also:

console.log(__resourceQuery);

@SimenB
Copy link
Contributor

SimenB commented Sep 18, 2015

Using

console.log(__resourceQuery);
console.log(urlParts);
console.log(window.location.hostname);
console.log(
  url.format({
    protocol: urlParts.protocol,
    auth: urlParts.auth,
    hostname: (urlParts.hostname === '0.0.0.0') ? window.location.hostname : urlParts.hostname,
    port: urlParts.port
  })
);

Linux (working):
image

Windows (failing):
image

@syranide
Copy link
Contributor Author

__resourceQuery is obviously all wrong on windows for some reason. So this is related to some weird manipulation by webpack or quirk in your config.

@SimenB
Copy link
Contributor

SimenB commented Sep 18, 2015

Oh seems to be Lync messing up the copy paste... Using 0.0.0.0 works!
Lync added the url again in <>, which messed up the parsing.

I still think url.parse messes up, but this works for us now 😄

@syranide
Copy link
Contributor Author

I still think url.parse messes up, but this works for us now 😄

It parses valid URLs correctly, so as far as we should be concerned it behaves as intended.

@mingfang
Copy link

can we get port to be like this?

port: urlParts.port ? urlParts.port : window.location.port

@jedwards1211
Copy link

@SimenB would the approach you guys are discussing work when connecting to your dev machine from other devices?

To accomplish that when I need to change the port I've been using this hack:

  <script type="text/javascript">
    var scriptElem = document.createElement('script');
    scriptElem.type = 'text/javascript';
    scriptElem.src = new RegExp('https?://[^:/]+').exec(window.location.href)[0] + ':9090/assets/client.bundle.js';
    document.head.appendChild(scriptElem);
  </script>

@SimenB
Copy link
Contributor

SimenB commented Oct 7, 2015

Yeah, connecting from other machines works fine, as long as they're on the same LAN. Might work independently of that, but we haven't tested it

@jedwards1211
Copy link

@SimenB What do your script tags look like? I've been curious myself if there's a better way

@SimenB
Copy link
Contributor

SimenB commented Oct 7, 2015

I use html-webpack-plugin, so no manual script tags.

Relevant part of template:

<body>
<div id="container"></div>
{{#each htmlWebpackPlugin.files.chunks}}
    <script src="{{this.entry}}"></script>
{{/each}}
</body>

My entry for webpack can be seen in a comment above

@jedwards1211
Copy link

Oh, are you using Meteor?

@SimenB
Copy link
Contributor

SimenB commented Oct 9, 2015

No, it's Backbone app, but the templates are using Handlebars, so I kept going with that for html-webpack-plugin (as I really dislike blueimp). Next version of the plugin will support lodash templates though, so we'll probably switch to that.

@jedwards1211
Copy link

I see. But now you're just using http://0.0.0.0:3030... for the src links of the scripts? I didn't realize this would work

@SimenB
Copy link
Contributor

SimenB commented Oct 9, 2015

The script tags are generated, and are the entry chunks (of which we have 1).

image

No specifying the port in scripts, as webpack-dev-server handles that part

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.

6 participants