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

refactor!: align preview api #5407

Merged
merged 2 commits into from Oct 27, 2021
Merged

refactor!: align preview api #5407

merged 2 commits into from Oct 27, 2021

Conversation

patak-dev
Copy link
Member

Description

We exposed the preview as experimental in #5014. SvelteKit didn't end up using it, so we are still able to review this API without downstream issues before stabilizing the feature.

We have also been merging some PRs that added options in the second parameter (initially it was only the port), and others that used the external inline config (open, https, strictPort). We now have some in both places (host, after #5389, that we merged to fix the logging of URLs as this was using the config instead of the host in the second parameter options of preview).

This PR aligns the preview API with the createServer and build functions. In both cases, they take an InlineConfig and do the resolution of the config internally. The method also returns a new PreviewServer object that exposes the config, httpServer and printUrls following the same pattern we use for ViteDevServer. This would also let us extend the preview server in the future if needed.

I'm sending the PR so we can discuss details about the API. Options:

  1. Current PR: We are reusing userConfig.server and resolvedConfig.server following the current design. This is ok because users normally want to share the configuration for both servers (cors, https, proxy, etc). The only config that needs to be overwritten is port. This PR proposes that we should let the user change it using the inlineConfig.
  2. Same as 1, but we get back to passing the port (and just the port) as the second parameter. So preview(inlineconfig, port=5000). This will be closer to the original function.
  3. We create a new userConfig.preview: { port: 5000, ... } config option and a resolvedConfig.preview that has the resolved info for the preview server. This PR reorders the interface for the ServerOptions placing every Http Server Options on the top so it is easier to know what is shared between the dev and preview servers. We could have an HttpServerOptions interface that is shared and a new PreviewServerOptions. The preview server would default to the options in dev server for host, strictPort, https, cors, proxy (and maybe open) but not for port. This would allow the user to define the preview port in vite.config.js instead of in the package.json script. It could be more clear also to see in the ResolvedConfig the resolved HTTP server options for the dev server and the preview server separately.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@Shinigami92 Shinigami92 self-requested a review October 24, 2021 21:32
@Shinigami92 Shinigami92 added the p1-chore Doesn't change code behavior (priority) label Oct 25, 2021
@patak-dev patak-dev added p2-nice-to-have Not breaking anything but nice to have (priority) and removed p1-chore Doesn't change code behavior (priority) labels Oct 25, 2021
@patak-dev patak-dev requested a review from antfu October 25, 2021 08:28
Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

LGTM

@patak-dev patak-dev added this to the 2.7 milestone Oct 27, 2021
@patak-dev patak-dev changed the title refactor: align preview api refactor!: align preview api Oct 27, 2021
@patak-dev patak-dev merged commit 4edb336 into main Oct 27, 2021
@patak-dev patak-dev deleted the refactor/align-preview-api branch October 27, 2021 20:22
@ElMassimo
Copy link
Contributor

ElMassimo commented Oct 28, 2021

Just to summarize a discussion in Discord:

îles would benefit from this exposed preview API. Beta branch using 2.7.0-beta.0.

Vitepress would also be able to use this command in favor of its custom implementation of preview.

Not strictly related, but it would be nice for Vitepress to align its command names as well (preview instead of serve).


Having a preview option in vite.config sounds great (option number 3 in the PR description).

@patak-dev patak-dev mentioned this pull request Nov 2, 2021
4 tasks
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

4 participants