feat(hmr): #3093 add config.server.hmr.runtimePort option #3115

Sociosarbis · 2021-04-23T14:58:10Z

Description

fix #3093
add an option named runtimePort in config.server.hmr . The value of runtimePort will replace __HMR_PORT__ literal.

Additional context

Is this a good option design ? Is a test case needed?

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines.
Read the Pull Request Guidelines and follow the Commit Convention.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Ideally, include relevant tests that fail without this PR but pass with it.

packages/vite/src/node/plugins/clientInjections.ts

nihalgonsalves · 2021-04-24T11:33:15Z

docs/config/index.md

@@ -415,12 +415,14 @@ export default async ({ command, mode }) => {

 ### server.hmr

- **Type:** `boolean | { protocol?: string, host?: string, port?: number, path?: string, timeout?: number, overlay?: boolean }`
+- **Type:** `boolean | { protocol?: string, host?: string, port?: number, path?: string, timeout?: number, overlay?: boolean, runtimePort?: string }`


The port is a number, so we should stick to that:

Suggested change

- **Type:** `boolean | { protocol?: string, host?: string, port?: number, path?: string, timeout?: number, overlay?: boolean, runtimePort?: string }`

- **Type:** `boolean | { protocol?: string, host?: string, port?: number, path?: string, timeout?: number, overlay?: boolean, runtimePort?: number }`

Because runtimePort is resolved on the browser. runtimePort is meaningless when it's a number.

nihalgonsalves · 2021-04-24T11:39:45Z

packages/vite/src/node/plugins/clientInjections.ts

        let port
-        if (config.server.middlewareMode) {
-          port = String(
-            (typeof config.server.hmr === 'object' && config.server.hmr.port) ||
-              24678
-          )
-        } else {
-          port = String(options.port || config.server.port!)
-        }
-        let hmrBase = config.base
-        if (options.path) {
-          hmrBase = path.posix.join(hmrBase, options.path)
-        }
-        if (hmrBase !== '/') {
-          port = path.posix.normalize(`${port}${hmrBase}`)
+        if (!options.runtimePort) {
+          if (config.server.middlewareMode) {
+            port = String(
+              (typeof config.server.hmr === 'object' &&
+                config.server.hmr.port) ||
+                24678
+            )
+          } else {
+            port = String(options.port || config.server.port!)
+          }
+          let hmrBase = config.base
+          if (options.path) {
+            hmrBase = path.posix.join(hmrBase, options.path)
+          }
+          if (hmrBase !== '/') {
+            port = path.posix.normalize(`${port}${hmrBase}`)
+          }
        }


With this logic we're losing appending the hmrBase Was that intended?

It's also less readable to move the runtimePort check until later.

This would retain the behaviour:

let port if (options.runtimePort != null) { port = runtimePort } else if (config.server.middlewareMode) { port = String( (typeof config.server.hmr === 'object' && config.server.hmr.port) || 24678, ) } else { port = String(options.port || config.server.port!) } let hmrBase = config.base if (options.path) { hmrBase = path.posix.join(hmrBase, options.path) } if (hmrBase !== '/') { port = path.posix.normalize(`${port}${hmrBase}`) }

Thanks for pointing this out. I will append hmrBase in my next commit.

vite/packages/vite/src/node/plugins/clientInjections.ts

Lines 46 to 49 in 07dbae0

runtimePort = options.runtimePort

if (hmrBase !== '/') {

runtimePort += ` + ${JSON.stringify(path.posix.normalize(hmrBase))}`

}

I have gotten hmrBase back.

nihalgonsalves · 2021-04-24T11:40:05Z

packages/vite/src/node/plugins/clientInjections.ts

@@ -47,7 +50,7 @@ export function clientInjectionsPlugin(config: ResolvedConfig): Plugin {
          .replace(`__DEFINES__`, serializeDefine(config.define || {}))
          .replace(`__HMR_PROTOCOL__`, JSON.stringify(protocol))
          .replace(`__HMR_HOSTNAME__`, JSON.stringify(host))
-          .replace(`__HMR_PORT__`, JSON.stringify(port))
+          .replace(`__HMR_PORT__`, options.runtimePort ?? JSON.stringify(port))


With the changes above, this no longer needs to be changed:

Suggested change

.replace(`__HMR_PORT__`, options.runtimePort ?? JSON.stringify(port))

.replace(`__HMR_PORT__`, JSON.stringify(port))

nihalgonsalves · 2021-04-24T11:43:44Z

docs/config/index.md


  Disable or configure HMR connection (in cases where the HMR websocket must use a different address from the http server).

  Set `server.hmr.overlay` to `false` to disable the server error overlay.

+  Set `server.hmr.runtimePort` to any runtime code which interpreted as a port. e.g. `'window.location.port'` and `(() => window.location.port)()`. If this is set, `server.hmr.port` will be ignored.


This would not work, would it? The config is being resolved on the server/in the plugin, not on the browser.

Suggested change

Set `server.hmr.runtimePort` to any runtime code which interpreted as a port. e.g. `'window.location.port'` and `(() => window.location.port)()`. If this is set, `server.hmr.port` will be ignored.

Set `server.hmr.runtimePort` to the port that the client should connect to. This is useful when a connection is proxied and the external port differs from the `server.hmr.port`.

runtimePort is useful when port can't be determined in advance. As mentioned in [#3093]#3093, the port of proxy is generated randomly. runtimePort is port resolved in runtime.

Right, I misunderstood. Edited my comment here to continue the discussion: #3115 (comment)

nihalgonsalves · 2021-04-24T11:59:18Z

If the goal is to resolve #3093, you'd have to define something directly here:

https://github.com/vitejs/vite/blob/c0a0a8427e61dc0f5774056aebd038c69492294a/packages/vite/src/client/client.ts

I'm wondering whether a static port is sufficient though.

Edit: I now understand that you proposed to add an inline code snippet that's replaced as a string. I'm not sure if this is the right approach, as it's a bit of a surprising configuration method. Thoughts @patak-js @Shinigami92?

Shinigami92 · 2021-04-24T15:23:46Z

Yeah, I'm not sure about this, but also not really biased 🤔
If it helps it helps, but I would also prefer a real callback or something like that instead of a stringyfied version of a callback/variable
Anyways, this is kinda a niche feature

jinghm318 · 2021-05-07T03:49:21Z

@nihalgonsalves @Sociosarbis So how's it now? I'm eager to get this feature.

#3011

antfu · 2021-05-07T04:22:51Z

Related #2921

Shinigami92 · 2021-05-28T14:15:08Z

We decided that #3578 is the successor of this PR

johnnysprinkles · 2021-05-28T17:29:41Z

Apologies, I did a bad job of searching for existing PRs! I'm making suggested changes in that #3578 today.

Sociosarbis added 2 commits April 23, 2021 22:20

feat(hmr): vitejs#3093 add config.server.hmr.runtimePort option

162f090

docs(hmr): add caution for runtimePort option

a2adcc3

Shinigami92 reviewed Apr 23, 2021

View reviewed changes

packages/vite/src/node/plugins/clientInjections.ts Outdated Show resolved Hide resolved

Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Apr 23, 2021

chore(hmr): simplify with double question mark

2c71e02

Shinigami92 previously approved these changes Apr 24, 2021

View reviewed changes

nihalgonsalves suggested changes Apr 24, 2021

View reviewed changes

fix(hmr): append hmrBase

07dbae0

Sociosarbis dismissed Shinigami92’s stale review via 07dbae0 April 24, 2021 14:20

Shinigami92 added the feat: hmr label Apr 24, 2021

coolhome mentioned this pull request May 3, 2021

example of setup with vite istead of vuecli? EEParker/aspnetcore-vueclimiddleware#93

Open

antfu mentioned this pull request May 28, 2021

feat: added clientPort to HmrOptions #3578

Merged

9 tasks

Shinigami92 closed this May 28, 2021

Shinigami92 added the duplicate This issue or pull request already exists label May 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(hmr): #3093 add config.server.hmr.runtimePort option #3115

feat(hmr): #3093 add config.server.hmr.runtimePort option #3115

Sociosarbis commented Apr 23, 2021 •

edited by antfu

Loading

nihalgonsalves Apr 24, 2021

Sociosarbis Apr 24, 2021

nihalgonsalves Apr 24, 2021

Sociosarbis Apr 24, 2021

Sociosarbis Apr 24, 2021

nihalgonsalves Apr 24, 2021

nihalgonsalves Apr 24, 2021

Sociosarbis Apr 24, 2021

nihalgonsalves Apr 24, 2021

nihalgonsalves commented Apr 24, 2021 •

edited

Loading

Shinigami92 commented Apr 24, 2021

jinghm318 commented May 7, 2021 •

edited

Loading

antfu commented May 7, 2021

Shinigami92 commented May 28, 2021

johnnysprinkles commented May 28, 2021

	- Type: `boolean \| { protocol?: string, host?: string, port?: number, path?: string, timeout?: number, overlay?: boolean, runtimePort?: string }`
	- Type: `boolean \| { protocol?: string, host?: string, port?: number, path?: string, timeout?: number, overlay?: boolean, runtimePort?: number }`

	runtimePort = options.runtimePort
	if (hmrBase !== '/') {
	runtimePort += ` + ${JSON.stringify(path.posix.normalize(hmrBase))}`
	}

	.replace(`__HMR_PORT__`, options.runtimePort ?? JSON.stringify(port))
	.replace(`__HMR_PORT__`, JSON.stringify(port))

	Set `server.hmr.runtimePort` to any runtime code which interpreted as a port. e.g. `'window.location.port'` and `(() => window.location.port)()`. If this is set, `server.hmr.port` will be ignored.
	Set `server.hmr.runtimePort` to the port that the client should connect to. This is useful when a connection is proxied and the external port differs from the `server.hmr.port`.

feat(hmr): #3093 add config.server.hmr.runtimePort option #3115

feat(hmr): #3093 add config.server.hmr.runtimePort option #3115

Conversation

Sociosarbis commented Apr 23, 2021 • edited by antfu Loading

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nihalgonsalves commented Apr 24, 2021 • edited Loading

Shinigami92 commented Apr 24, 2021

jinghm318 commented May 7, 2021 • edited Loading

antfu commented May 7, 2021

Shinigami92 commented May 28, 2021

johnnysprinkles commented May 28, 2021

Sociosarbis commented Apr 23, 2021 •

edited by antfu

Loading

nihalgonsalves commented Apr 24, 2021 •

edited

Loading

jinghm318 commented May 7, 2021 •

edited

Loading