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

unify preview server config #11289

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

unify preview server config #11289

wants to merge 4 commits into from

Conversation

oe
Copy link

@oe oe commented Jun 19, 2024

Changes

Merge preview server config by:

const previewConfig = Object.assign({},
  settings.config.vite.server,
  settings.config.vite.preview,
  settings.config.server,
)

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

  1. add proxy config to astro.config.ts
  2. build your project
  3. run astro preview and check network

Docs

This PR adds new feature to astro preview, docs should be updated

Copy link

changeset-bot bot commented Jun 19, 2024

🦋 Changeset detected

Latest 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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) semver: major Change triggers a `major` release labels Jun 19, 2024
Copy link
Contributor

@github-actions github-actions bot left a 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
Copy link
Member

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?

Copy link
Author

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
Copy link
Member

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

Copy link
Author

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.

@ematipico
Copy link
Member

This PR adds new feature to astro preview, docs should be updated

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?

Comment on lines +69 to +73
const previewConfig = Object.assign({},
settings.config.vite.server,
settings.config.vite.preview,
settings.config.server,
)
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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.

Copy link
Author

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.

@oe
Copy link
Author

oe commented Jun 20, 2024

This PR adds new feature to astro preview, docs should be updated

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?

The Server Options may need update according to Vite Preview Options

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) semver: major Change triggers a `major` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants