-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
unify preview server config #11289
base: main
Are you sure you want to change the base?
unify preview server config #11289
Conversation
🦋 Changeset detectedLatest commit: 3feb8cd 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 |
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.
This PR is blocked because it contains a major
changeset. A reviewer will merge this at the next release if approved.
@@ -0,0 +1,5 @@ | |||
--- | |||
'astro': major |
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.
Why is this a major
?
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 don't know, after running pnpm exec changeset
, major
is the only option left
'astro': major | ||
--- | ||
|
||
unify preview server config |
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.
Can you leave a better explanation? Developers will read the changelog, and I doubt they will understand what "unify" means
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.
A more accurate description would be to keep Astro Preview behaving the same way as Vite Preview, supporting Vite Preview settings.
According to the Astro code, static previews are supported by default, but third-party adapter previews require additional support.
Developers are usually in charge of updating the docs. Could you send a PR to update the docs? Or at least, which docs should be updated? |
const previewConfig = Object.assign({}, | ||
settings.config.vite.server, | ||
settings.config.vite.preview, | ||
settings.config.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.
I don't think this is correct. If you want to configure the Astro preview server, you should use the config.server
config. Not every Vite options should take effect when a framework builds on top of it.
Also, settings.config.vite.server
is for the dev server, and settings.config.vite.preview
is for the preview server. We can't merge it like this.
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.
According to the official Vite documentation, vite preview
uses the Vite Server configuration by default. Astro is built on Vite, but doesn't use Vite's Server configuration, which is very strange. (The proxy configuration in Dev and Preview is very useful for this purpose.
Anyway, the above code just keeps Astro Preview behavior consistent with Vite Preview, and is technically a bug fix.
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.
Astro preview does use the Vite preview server internally, but we don't document that the Vite server/preview options will strictly work when configured. Editing this part of the code is also sensitive because you're passing all the Vite-specific server/preview options to previewModule.default
directly.
We're currently intentionally limiting the options here because it would be expected for all adapters to support the same set of features, but not all adapters use the Vite preview server. That way, the adapters need to only concern of supporting features from config.server
.
If this is about the proxy
config, it leans towards a feature request, not a bug fix. I could see perhaps passing that only to previewModule.default
and update the types to be optional, and adapters could decide to support it. But I'd like to hear what others think of this.
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.
Got it. What I need is proxy
, it can be approached by configuring vite.server on astro dev, but doesn't work on astro preview.
The Server Options may need update according to Vite Preview Options |
@oe are you still interested in this PR? |
Yes, what do I need to do? |
@bluwy do you have advices? |
Changes
Merge preview server config by:
Make the preview server can access all server config from
vite.server
/vite.preview
/server
.Especially, this make preview server access
proxy
settings from these objects which allow preview proxy api request just like dev server!Testing
astro.config.ts
astro preview
and check networkDocs
This PR adds new feature to
astro preview
, docs should be updated