Skip to content

Commit

Permalink
fix: fix hmr in docker + support devServer.public with protocol
Browse files Browse the repository at this point in the history
  • Loading branch information
@yyx990803
yyx990803 committed Jul 27, 2018
1 parent 7d65353 commit da38ed4
Showing 1 changed file with 11 additions and 11 deletions.
22 changes: 11 additions & 11 deletions packages/@vue/cli-service/lib/commands/serve.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ module.exports = (api, options) => {
const isProduction = process.env.NODE_ENV === 'production'

const path = require('path')
const url = require('url')
const chalk = require('chalk')
const webpack = require('webpack')
const WebpackDevServer = require('webpack-dev-server')
Expand Down Expand Up @@ -69,7 +68,12 @@ module.exports = (api, options) => {
const host = args.host || process.env.HOST || projectDevServerOptions.host || defaults.host
portfinder.basePort = args.port || process.env.PORT || projectDevServerOptions.port || defaults.port
const port = await portfinder.getPortPromise()
const publicUrl = args.public || projectDevServerOptions.public
const rawPublicUrl = args.public || projectDevServerOptions.public
const publicUrl = rawPublicUrl
? /^[a-zA-Z]+:\/\//.test(rawPublicUrl)
? rawPublicUrl
: `${protocol}://${rawPublicUrl}`
: null

const urls = prepareURLs(
protocol,
Expand All @@ -85,16 +89,12 @@ module.exports = (api, options) => {

// inject dev & hot-reload middleware entries
if (!isProduction) {
const sockjsUrl = publicUrl ? `//${publicUrl}/sockjs-node` : url.format({
protocol,
port,
hostname: urls.lanUrlForConfig || 'localhost',
pathname: '/sockjs-node'
})

const sockjsUrl = publicUrl
? `?${publicUrl}/sockjs-node`
: ``
const devClients = [
// dev server client
require.resolve(`webpack-dev-server/client`) + `?${sockjsUrl}`,
require.resolve(`webpack-dev-server/client`) + sockjsUrl,
// hmr client
require.resolve(projectDevServerOptions.hotOnly
? 'webpack/hot/only-dev-server'
Expand Down Expand Up @@ -183,7 +183,7 @@ module.exports = (api, options) => {
}

const networkUrl = publicUrl
? (protocol + '://' + publicUrl).replace(/([^/])$/, '$1/')
? publicUrl.replace(/([^/])$/, '$1/')
: urls.lanUrlForTerminal
console.log()
console.log([
Expand Down

7 comments on commit da38ed4

@drubin
Copy link

@drubin drubin commented on da38ed4 Dec 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yyx990803 thanks for this I am wondering if we can maybe take the same approach that we did with gatsby here? https://github.com/gatsbyjs/gatsby/pull/9734/files

I am having issue trying to set this to / but because of the relative regex requiring a : it doesn't match so I am forced to put an absolute path here.

@jkzing
Copy link
Member

@jkzing jkzing commented on da38ed4 Dec 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drubin, why would you want to use / in devServer.public option, isn't baseUrl option fit for you?

@drubin
Copy link

@drubin drubin commented on da38ed4 Dec 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkzing Thanks for quick reply.

The full discussion can be found gatsbyjs/gatsby#8348 (comment) I had the same issue with Gatsbyjs (underlying webpack library) so I think applying similar logic here would make sense.

The summary is that when building demos or reusable configs you often don't know what the public endpoint/ip will be and users might want a custom domain name.

Gatsby originally defaulted the HMR url to --host similar to --public param which seemed like a good default unless you are running behind docker/vagrant/proxy server of some kind. In which case the IP address you bind to on your local machine won't always be the same DNS name that the web browser will access it from.

I think this might solve many of the IP/host issues around docker/vagrant/proxies as it did with Gatsby.

Let's take from example a simple setup.

You run Vue CLI inside docker (shareable configs and easier to get started) the docker containers IP is 192.168.1.20. Your vue configuration would be vue-cli-service serve --host=0.0.0.0 ---public=http://192.168.1.20/ so it binds to the public IP address but now the HMR endpoint defaults to the http://192.168.1.20/? now you restart your docker container and the IP address changes to 192.168.1.21.. so you have to change the configuration again if you were able to default it to / you would just be able to restart it.

In the PR above we changed the default endpoint to allow relative pathing i.e / as a default so that when you change configuration dns/ip you don't need to also modify your configuration and rebuild/compile.

Because of the regex check here, I am unable to leave off the port + hostname and pass a configuration like --public=/ as it automatically replaces it to --protocol and --public

I am not sure where it came from but you can pass relative paths into require.resolve(webpack-dev-server/client) + sockjsUrl, as shown but the Gatsby PR.

Hope this explains it clearly if not please let me know and I will show you an example.

So even if you don't change the default. Would you be open to accepting a PR removing the regex check to allow purely relative URLs?

@jkzing
Copy link
Member

@jkzing jkzing commented on da38ed4 Dec 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drubin Understood.

The current behavior we used to infer sockjs url is to support use case that dev-server is started behind a reverse proxy (see #1472 (comment)). As we have a baseUrl option in vue.config.js, devServer.public may not be able to defaults to /. (Because of devServer.public having higher priority than baseUrl).

But I would vote yes to make devServer.public accept relative urls.

@drubin
Copy link

@drubin drubin commented on da38ed4 Dec 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkzing I guess what I am trying to say is the sockjs url can also be relative, and that's exactly what Gatsbyjs did.

I think a while ago it was required to be absolute but it seems to work in every possible case we have tried (see full list under Gatsby's PR).

Because the code runs in the browser and the browser will always automatically default to using the correct name of the "public dns" you accessed the page from if the default is a relative path?

I see that point but if we let the browser decide on the base path (domain / port / protocol) it would solve that problem as well (same exact issue that Gatsby had)

@drubin
Copy link

@drubin drubin commented on da38ed4 Dec 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that example, the custom configuration doesn't actually make sense as I previously stated (they might have only done this because it didn't work with defaults? (that's my assumption)).

The assumption here is that the --host param can be used as a URL, IP address binding should not be the same configuration as public url's .

const path = require('path')

const host = '0.0.0.0'
const port = 8085

module.exports = {
  lintOnSave: false,
  baseUrl: `http://${host}:${port}/`,

@jkzing
Copy link
Member

@jkzing jkzing commented on da38ed4 Dec 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drubin

I think a while ago it was required to be absolute but it seems to work in every possible case we have tried (see full list under Gatsby's PR).

I didn't find the cases you said in the PR. But even if it works in Gatsby, it doesn't mean also works in vue CLI. (Gatsby is using webpack-hot-middleware/client and vue CLI is using webpack-dev-server/client, they are different implementations.)

Because the code runs in the browser and the browser will always automatically default to using the correct name of the "public dns" you accessed the page from if the default is a relative path?

No, at least with webpack-dev-server/client it can not. See how webpack-dev-server/client do this here: https://github.com/webpack/webpack-dev-server/blob/fadae5da6ba0261cade08164feeaad99b1de6b79/client-src/default/index.js#L30

In that example, the custom configuration doesn't actually make sense as I previously stated (they might have only done this because it didn't work with defaults? (that's my assumption)).

This can be turned to "Whether accessing index.html from a host other than the one dev-server uses makes sense". From my experience, it makes sense.

If you still think it's a better idea to default to relative path, let's open an issue and discuss it there.😊

Please sign in to comment.