Default preview host to localhost #5753
Conversation
🦋 Changeset detectedLatest commit: 0ce0cb1 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
pkg: astro
| const onError = (err: NodeJS.ErrnoException) => { | ||
| if (err.code && err.code === 'EADDRINUSE') { | ||
| if (!showedPortTakenMsg) { | ||
| info(logging, 'astro', msg.portInUse({ port })); |
There was a problem hiding this comment.
This PR doesn't log port in use anymore as that's deferred to the Vite preview server, which also has it's own logging.
There was a problem hiding this comment.
can you share a screenshot or video of what this looks like?
There was a problem hiding this comment.
It should look something like this!
> @example/basics@0.0.1 preview /Users/bjorn/Work/oss/astro/examples/basics
> astro preview
Port 3000 is in use, trying another one...
Port 3001 is in use, trying another one...
🚀 astro v2.0.0-beta.0 started in 137ms
┃ Local http://127.0.0.1:3002/
┃ Network use --host to expose
I've made 3000 and 3001 occupied, so it'll log two lines that it's trying another one. Previously here's how it looks:
> @example/basics@0.0.1 preview /Users/bjorn/Work/oss/astro/examples/basics
> astro preview
10:40:28 [astro] Port 3000 in use. Trying a new one…
🚀 astro v2.0.0-beta.0 started in 2ms
┃ Local http://localhost:3002/
┃ Network use --host to expose
(Note: the 127.0.0.1 and localhost change is part of the main change of this PR to align with dev)
| @@ -1,11 +1,19 @@ | |||
| export function getResolvedHostForHttpServer(host: string | boolean) { | |||
| if (host === false) { | |||
| // Use a secure default | |||
| return '127.0.0.1'; | |||
| return 'localhost'; | |||
There was a problem hiding this comment.
This is the main change to align to the Vite dev server. In nodejs <17, this defaults to 172.0.0.1. In nodejs >=17, this defaults to [::1]. So this should allow the preview server to serve in ipv6 too.
| export function vitePluginAstroPreview(settings: AstroSettings): Plugin { | ||
| const { base, outDir, trailingSlash } = settings.config; | ||
|
|
||
| return { | ||
| name: 'astro:preview', | ||
| apply: 'serve', | ||
| configurePreviewServer(server) { |
There was a problem hiding this comment.
This plugin is where I moved the initial custom middleware handling into. Including validating base and trailingSlash config. Plus a fallback 404.html handling.
|
Would love to get a review from @FredKSchott on this one as I believe he reworked the preview server a while back. Is there any downside to using the Vite preview server? |
There was a problem hiding this comment.
LGTM with one comment about making sure that logging makes sense and doesn't sneak in any vite-isms that would confuse the user. Behavior-wise, Vite's preview server is what we were trying to emulate so this change makes sense to me.
Also, @bluwy have you confirmed that this works on a larger site? If not, please run build + preview on the docs site to confirm that this works before merging. Thanks!
Good call. I spinned up the docs site locally and everything seems to be working/loading fine after clicking around. |
|
Seems like there might be a legitimate issue with prefetching on Windows according to CI. Marking as draft. |
github-actions
bot
added
the
pkg: integration
Changes
Fix #5747
localhostinstead of127.0.0.1.vite.preview. It has the added benefit of simpler URL logging, enforced case-sensitivity when serving files, and alignment with dev server behaviour in general.Testing
Existing tests should pass. There's a preview routing test that I find quite extensive. I've also tested manually with the example projects.
Docs
I think this change doesn't affect usability in practice so we don't need to highlight this behaviour in the docs.