-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Support Vite 5 #9122
Support Vite 5 #9122
Conversation
🦋 Changeset detectedLatest commit: 734a1f7 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 |
// @ts-expect-error there's a bug with the types where the first arg should be optional | ||
resolvedOptions.preprocess = vitePreprocess(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've sent a PR upstream to fix it. If it's released, I'll update this part before merging.
// Vite doesn't handle /foo/ if /foo.html exists, we handle it anyways | ||
if (pathname.endsWith('/')) { | ||
const pathnameWithoutSlash = pathname.slice(0, -1); | ||
const htmlPath = fileURLToPath(outDir + pathnameWithoutSlash + '.html'); | ||
if (fs.existsSync(htmlPath)) { | ||
req.url = pathnameWithoutSlash + '.html'; | ||
return next(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment above should clarify what this does. But to further explain why we're assigning to req.url
, this is for a later Vite middleware to load the files. We only need to update the req.url
so that middleware can detect a HTML file.
@@ -72,8 +72,6 @@ export default async function createStaticPreviewServer( | |||
host: getResolvedHostForHttpServer(settings.config.server.host), | |||
port: settings.config.server.port, | |||
closed, | |||
// In Vite 5, `httpServer` may be a `Http2SecureServer`, but we know we are only starting a HTTP server | |||
// eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion | |||
server: previewServer.httpServer as http.Server, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a world where we'd want to support https for the preview server? Unrelated to this, just wondering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could! At the moment we have control over the config passed to Vite's preview()
so we know this will always be a http server. If we open up to the config from astro.config.mjs
in the future, they can create https servers.
@@ -1,4 +1,4 @@ | |||
import { defineConfig } from 'rollup' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @bluwy 🚀
Changes
vite-plugin-astro-preview
to handle certain URLs that Vite 5 doesn't handle anymorePS: I'm not sure why pnpm updated te lockfile to be larger. It should be fixed when we bump all the deps to latest later.
Testing
Tested manually. Ran all tests locally which passed.
Docs
I think we only need to mention this in the migration guide.