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

fix(hmr): respect server https options when running as middleware #1992

Merged
merged 1 commit into from
May 10, 2021

Conversation

airhorns
Copy link
Contributor

If you are running vite over https, HMR works great in the standalone mode, but when in middleware mode, the middleware might be served over https and the websocket server wouldn't listen on https. This updates the websocket server to respect the config.server.https option when in middleware mode too.

  • Moves the https config into a top level key of the resolved config, so its only resolved once (and only one set of certs is generated) and can be shared between different consumers
  • Boots a secure websocket server when in middleware mode if the config.server https option is on

@airhorns
Copy link
Contributor Author

airhorns commented Feb 18, 2021

Congrats on 2.0 going GA! Would be real great to get this in -- anything more I should do?

Copy link
Member

@yyx990803 yyx990803 left a comment

Choose a reason for hiding this comment

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

Moves the https config into a top level key of the resolved config, so its only resolved once (and only one set of certs is generated) and can be shared between different consumers

I don't think this is necessary. resolveConfig is called for all commands including build and preview, so resolving https (which is dev server only) in resolveConfig is definitely not ideal.

The https options should be resolved in server/index.ts and then passed to resolveHttpServer and createWebSocketServer.

@airhorns
Copy link
Contributor Author

Ah, that totally makes sense, my bad. The reason I did it this way is because both those inner functions need access to the resolved config anyways, and resolveHttpServer is actually called in the src/node/serve.ts file as well so I didn't want to duplicate the https cert resolution logic, or run it twice, because generating the self signed certs is kind of expensive. Is there an easy way to run the https config resolution only for the commands that need it and keep the resolved value on ResolvedConfig? Or would you rather I just duplicate the resolution? Happy to do either just let me know!

@yyx990803
Copy link
Member

I think we can extract a resolveHttpsOptions method to be called in both server/index.ts and serve.ts. Then resovleHttpServer take the options via argument instead.

@airhorns
Copy link
Contributor Author

Ok, updated!

yyx990803
yyx990803 previously approved these changes Feb 24, 2021
@airhorns
Copy link
Contributor Author

Would love to get this in!

@airhorns
Copy link
Contributor Author

Rebased this on top of latest main

@airhorns
Copy link
Contributor Author

ping @yyx990803 anything else I can do to get this in?

@robots4life
Copy link

Referencing this issue cytopia/devilbox#797 and hope it will be sorted with this merge.

@Shinigami92 Shinigami92 added 🔍 review needed enhancement New feature or request p2-nice-to-have Not breaking anything but nice to have (priority) labels Mar 21, 2021
@robots4life
Copy link

@airhorns How can add your commit 1a96ed3 to "vite": "^2.1.0" locally ?

When I run ../node_modules/vite $ git checkout -b hmr-https 1a96ed32fde9f4821105c7ef846153ab92953967 locally I get fatal: reference is not a tree: 1a96ed32fde9f4821105c7ef846153ab92953967.

I assume this is because Vite is compiled so that the folder structure with the changed files in it is not there - meaning I cannot just copy and paste the changes in locally installed Vite.

Kinda wanted to give this a spin and see if that sorts things for the setup I use.

@Shinigami92
Copy link
Member

@robots4life
Copy link

@Shinigami92 I am not sure, from that info, but what I understand is, I should clone the entire monorepo and thus have the correct folders and files locally and then also add the changes locally by checking out the commit above in a branch named how I like, is that what you mean?

@Shinigami92
Copy link
Member

Shinigami92 commented Mar 23, 2021

Oh maybe I misunderstood something
I replied to your

[...] meaning I cannot just copy and paste the changes in locally installed Vite

assuming you currently working on a PR or so and just want to test something out

But maybe you can try https://www.npmjs.com/package/patch-package for now
But even with this, you may need to modify the emitted dist files and not the source code 🤔

@airhorns
Copy link
Contributor Author

airhorns commented Apr 27, 2021

Anyone available to help get this merged? There's another fix on top I would like to add that applies the same find-a-free-port logic that the main vite server has to the HMR port as well so you can start multiple instances if need be.

nihalgonsalves
nihalgonsalves previously approved these changes Apr 27, 2021
packages/vite/src/node/server/http.ts Outdated Show resolved Hide resolved
packages/vite/src/node/server/ws.ts Outdated Show resolved Hide resolved
packages/vite/src/node/server/ws.ts Show resolved Hide resolved
packages/vite/src/node/server/ws.ts Outdated Show resolved Hide resolved
packages/vite/src/node/server/http.ts Outdated Show resolved Hide resolved
packages/vite/src/node/server/ws.ts Outdated Show resolved Hide resolved
packages/vite/src/node/server/ws.ts Outdated Show resolved Hide resolved
@speigg
Copy link

speigg commented May 4, 2021

Any chance on having this merged soon, to fix the infinite redirect on mobile Safari?

@patak-dev
Copy link
Member

@airhorns would you merge main to this branch? We completed the migration to GitHub CI and tests should be green after you update the PR

If you are running vite over https, HMR works great in the standalone mode, but when in middleware mode, the middleware might be served over https and the websocket server wouldn't listen on https. This updates the websocket server to respect the config.server.https option when in middleware mode too.
@airhorns
Copy link
Contributor Author

airhorns commented May 4, 2021

Ok, updated to be on top of main and address the review! Also, I've been running this locally for a few months now and it's been working well!

@patak-dev patak-dev requested a review from antfu May 4, 2021 17:10
@antfu antfu merged commit 24178b0 into vitejs:main May 10, 2021
@airhorns airhorns deleted the hmr-https branch May 10, 2021 12:43
@robots4life
Copy link

robots4life commented May 19, 2021

SvelteKit now works with Devilbox, secure local dev domain and HMR!

svelte.config.js

import { readFileSync } from 'fs';

/** @type {import('@sveltejs/kit').Config} */
const config = {
	kit: {
		// hydrate the <div id="svelte"> element in src/app.html
		target: '#svelte',
		vite: () => ({
			server: {
				host: '0.0.0.0',
				port: 3000,
				https: {
					key: readFileSync('/shared/httpd/devilbox/ca/certs/mass/skdevkit.loc.key'),

					cert: readFileSync('/shared/httpd/devilbox/ca/certs/mass/skdevkit.loc.crt')
				},
				hmr: {
					host: 'skdevkit.loc',
					protocol: 'wss',
					port: 24678
				}
			}
		})
	}
};
export default config;

npm run dev -- --host 0.0.0.0

Thank you!

@JBusillo
Copy link
Contributor

@airhorns -- I've been researching SvelteKit HMR issues over HTTPS. SvelteKit operates in Vite's middleware mode. Even with your change, we're still experiencing constant refreshing when using self-signed certificates.

There's kind of an ugly workaround explained here.

It also works with a proposed fix to SvelteKit, but requires using an undocumented option (server.hmr.server), passing the reference of our HTTPS server. Although this works like a charm, there's hesitancy about using an option that isn't in the documentation.

Could you share your configuration for using HTTPS and HMR for unsigned certificates in middleware mode?

Thanks in advance.

@airhorns
Copy link
Contributor Author

airhorns commented May 29, 2021

I use signed certificates generated with mkcert so I am not sure exactly how to get unsigned working! I just pass the HTTPS config to the vite options:

{
  server: { 
    https: {
      key: fs.readFileSync(path.join(...)),
      cert: fs.readFileSync(path.join(...)),
    }
  }
}

I think the browser might be aborting your connection to the self signed cert unless you have convinced it to accept it?

@JBusillo
Copy link
Contributor

Yes, that is the issue with self-signed certificates -- The wss:// connection automatically uses https:// to authorize, but this process isn't associated with a browser window in order to accept it.

It works perfectly when I pass a reference to SvelteKit's created server via the vite.server.hmr.server option, but this parameter isn't documented on Vite's website. The initial https:// request (for the web socket) is sent to that server, the "Your connection is not private" dialog is presented once, and, after acceptance, allows the both the web app and hmr to proceed.

I think I'll create an issue for updating the documentation, and see what the "powers-that-be" decide.

I'm somewhat new to Github and open-source projects, so I'm a bit unfamiliar with the process. In any event, thanks for your reply -- I wanted to confirm whether your original issue addressed self signed certificates for middlewareMode setups.

@robots4life
Copy link

@JBusillo

Yes, that is the issue with self-signed certificates -- The wss:// connection automatically uses https:// to authorize, but this process isn't associated with a browser window in order to accept it.

It works perfectly when I pass a reference to SvelteKit's created server via the vite.server.hmr.server option, but this parameter isn't documented on Vite's website. The initial https:// request (for the web socket) is sent to that server, the "Your connection is not private" dialog is presented once, and, after acceptance, allows the both the web app and hmr to proceed.

@svelte/kit@next: 1.0.0-next.178.
Could you provide an example configuration where you reference the SvelteKit's created server via the vite.server.hmr.server option? Through Docker HMR unfortunately reloads, [vite] connecting...

vite: {
	server: {
		host: '0.0.0.0',
		port: 3000,
		// ??
		// https: {
		// 	key: readFileSync('app.loc.key'),
		// 	cert: readFileSync('app.loc.crt')
		// }
	},
	hmr: {
		protocol: 'wss',
		host: 'app.loc',
		port: 443,
		server: {
			host: '??',
			port: '??',
			// ?? 
			https: {
				key: readFileSync('app.loc.key'),
				cert: readFileSync('app.loc.crt')
			}
		}
	}
}

@robots4life
Copy link

robots4life commented Oct 5, 2021

sveltejs/kit#1134

Using Devilbox - Docker remote environment, it works with this simple config, running npm run dev -- --https --host 0.0.0.0 and going to https://app.loc:3000 instead of just https://app.loc (there I get a 403).
Also for the generated cert from Vite for localhost an exception has to be allowed once.

/** @type {import('@sveltejs/kit').Config} */
const config = {
	kit: {
		// hydrate the <div id="svelte"> element in src/app.html
		target: '#svelte',
		vite: {
			server: {
				host: '0.0.0.0'
			}
		}
	}
};

export default config;

What confused me was that SvelteKit did work just fine with HMR under https://app.loc/ without having a port number in the URL in the past.
So a change in the way HRM is done must/could have been introduced in Vite or SvelteKit.

Being able to tell Vite to use the cert from https://app.loc instead of making a new one for localhost on start could possibly fix this issue. There is an option for vite.server.hmr.server where I assume this could be solved however the option does not seem to work with a self signed cert so far.

import { readFileSync } from 'fs';

/** @type {import('@sveltejs/kit').Config} */
const config = {
	kit: {
		// hydrate the <div id="svelte"> element in src/app.html
		target: '#svelte',
		vite: {
			server: {
				host: '0.0.0.0'
			},
			hmr: {
				server: {
					https: {
						key: readFileSync('app.loc.key'),
						cert: readFileSync('app.loc.crt')
					}
				}
			}
		}
	}
};

export default config;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feat: hmr p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants