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

fix: --https is invalid in preview #4318

Merged
merged 1 commit into from Jul 20, 2021
Merged

Conversation

y1d7ng
Copy link
Contributor

@y1d7ng y1d7ng commented Jul 19, 2021

Description

fix #4169
--https is invalid in vite preview.

>  vite preview --https

console:

> Local: http://localhost:5000/

The protocol of the url is http, and the url with https https://localhost:5000 cannot be opened.

Additional context

options.https is not passed into resolveConfig

const config = await resolveConfig(
{
root,
base: options.base,
configFile: options.config,
logLevel: options.logLevel,
server: {
open: options.open,
strictPort: options.strictPort
}

but subsequent execution depends on config.server.https

const protocol = options.https ? 'https' : 'http'

export async function resolveHttpsConfig(
config: ResolvedConfig
): Promise<HttpsServerOptions | undefined> {
if (!config.server.https) return undefined


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

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

I think this would be a feature request 🤔
But ... do we even want to support that or is it by design?
preview should not serve content for production but just to test if the build/dist works

@y1d7ng
Copy link
Contributor Author

y1d7ng commented Jul 20, 2021

I think this would be a feature request 🤔
But ... do we even want to support that or is it by design?
preview should not serve content for production but just to test if the build/dist works

@Shinigami92 Sorry, maybe I didn't describe the pr clearly. The main solution of this pr is that the --https config option doesn't take effect when use preview. I think using https in preview is may not a feature according to the previous code.

.option('--https', `[boolean] use TLS + HTTP/2`)

#4169 This issue reports two bugs, I think the serve root bug is due to incorrect usage. This pr solved the bug of https.

@y1d7ng y1d7ng changed the title fix: --https get ignored in preview fix: --https is invalid in preview Jul 20, 2021
@patak-dev patak-dev merged commit a870584 into vitejs:main Jul 20, 2021
@husayt
Copy link
Contributor

husayt commented Aug 12, 2021

@patak-js @OneNail thanks, but this doesn't solve #4169 entirely.
Yes it solves --https parameter, but root is still not working with preview. I can't reopen the original card, since commenting here.

Thanks

@y1d7ng
Copy link
Contributor Author

y1d7ng commented Aug 12, 2021

@patak-js @OneNail thanks, but this doesn't solve #4169 entirely.
Yes it solves --https parameter, but root is still not working with preview. I can't reopen the original card, since commenting here.

Thanks

The vite preview root you use, is the root an normal directory or a directory of vite project?

@husayt
Copy link
Contributor

husayt commented Aug 13, 2021

Normally preview uses dist folder, but I also have storybook-dist and sometimes want to run from that folder. Why to install another static server, when vite can do that.?

@y1d7ng
Copy link
Contributor Author

y1d7ng commented Aug 13, 2021

Normally preview uses dist folder, but I also have storybook-dist and sometimes want to run from that folder. Why to install another static server, when vite can do that.?

According to the current design, root refers to your vite project, not the dist directory generated by build.

@y1d7ng y1d7ng deleted the hotfix/4169 branch August 13, 2021 07:51
@husayt
Copy link
Contributor

husayt commented Aug 13, 2021

@OneNail if root setting doesn't work, then why is it there? If contents of my dist folder was in xyzdist and I would run vite preview xyzdist it still would not work. I might be trying to use if for different purpose, but the feature doesn't work to start with

@sasivarnan
Copy link

I also have the exact use case as @husayt. I am using lib mode of vite to build react component library.

I use vite builder for storybook - storybook-builder-vite.

And there isn't much to preview the contents of dist directory as it is a library. Ability to preview stroybook-static folder after building it would be super helpful.

@y1d7ng
Copy link
Contributor Author

y1d7ng commented Aug 18, 2021

@OneNail if root setting doesn't work, then why is it there? If contents of my dist folder was in xyzdist and I would run vite preview xyzdist it still would not work. I might be trying to use if for different purpose, but the feature doesn't work to start with

The root setting can work. If the name of the vite project is xyzdist, you can preview by executing vite preview xyzdist outside this directory.

@y1d7ng
Copy link
Contributor Author

y1d7ng commented Aug 18, 2021

I also have the exact use case as @husayt. I am using lib mode of vite to build react component library.

I use vite builder for storybook - storybook-builder-vite.

And there isn't much to preview the contents of dist directory as it is a library. Ability to preview stroybook-static folder after building it would be super helpful.

You can consider opening a pr or issue for this feature.

aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 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.

--https and root get ignored during vite preview
5 participants