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

feat: preview config #5514

Merged
merged 6 commits into from
Nov 6, 2021
Merged

feat: preview config #5514

merged 6 commits into from
Nov 6, 2021

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Nov 2, 2021

Description

Continuation from #5407

We create a new userConfig.preview: { port: 5000, ... } config option and a resolvedConfig.preview that has the resolved info for the preview server. There is a new CommonServerOptions interface that is shared and a new PreviewServerOptions. The preview server defaults to the options in dev server for host, strictPort, https, cors, proxy, and open (I'm not sure about this last one), 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 is more clear to see in the ResolvedConfig the resolved HTTP server options for the dev server and the preview server separately.

This PR deprecates printHttpServerUrls because it doesn't work now that config.server and config.preview are separate in ResolvedConfig. It still works fine for printing the URLs from the server, that is what the ecosystem could be using this function for (as the experimental preview API was just exposed). Users of createServer and preview should use server.printUrls(). Maybe we should expose. The only place I see it being used is in Vitedge here https://github.com/frandiox/vitedge/blob/d64b6bc46988bf9df6896a32e499453d52476fbf/src/build/preview.js#L76, but this would continue to work.

src/node/server/http.ts has been moved to src/node/http.ts as it is shared between the dev server and the preview server


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@Shinigami92 Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Nov 3, 2021
@Shinigami92
Copy link
Member

@patak-js Is this really a feat? Or is it a refactor?

@patak-dev
Copy link
Member Author

It is a feat, it adds new config options. You can now specify the preview server port in vite.config.js

Shinigami92
Shinigami92 previously approved these changes Nov 4, 2021
@Shinigami92 Shinigami92 dismissed their stale review November 5, 2021 13:31

Add a test that uses the new preview property

@Shinigami92
Copy link
Member

Could you also please provide an example how such a new vite config would look like? 🙂

We should also think about that proxy and cors should be moved out from common to only dev config 🤔 But this should be done in another PR

yyx990803
yyx990803 previously approved these changes Nov 6, 2021
@Shinigami92 Shinigami92 added needs documentation Documentations are needed needs test labels Nov 6, 2021
@patak-dev
Copy link
Member Author

Added docs for preview. @Shinigami92 I checked how we could test the preview server and it isn't clear. We are currently using a custom static server during build test, maybe a good way to test it would be to replace it with the preview server. But this needs to be analyzed separately from this PR

function startStaticServer(): Promise<string> {

I'll merge this without extra tests at this point, test coverage remains the same as before this PR (we are only testing that the preview server starts correctly in the cli playground).

@Shinigami92 Shinigami92 removed the needs documentation Documentations are needed label Nov 6, 2021
@Shinigami92
Copy link
Member

Shinigami92 commented Nov 6, 2021

@patak-js What I would like to get tested is the preview property, so it get's read out from the config
Maybe it's possible with a simple unit test? Or maybe mocked?

@Niputi Niputi removed the needs test label Nov 6, 2021
@patak-dev patak-dev merged commit ff755eb into main Nov 6, 2021
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Dec 8, 2021
@antfu antfu deleted the feat/preview-config branch December 24, 2021 05:04
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.

4 participants