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: vite preview port is used automatically +1 #4219

Merged
merged 3 commits into from Jul 19, 2021

Conversation

ygj6
Copy link
Member

@ygj6 ygj6 commented Jul 12, 2021

Description

related #4185
vite preview port is used automatically +1.

If the port is specified, it will not +1

{
  "scripts": {
    "preview": "vite preview --port 8080"
  }
}

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 12, 2021
packages/vite/src/node/preview.ts Outdated Show resolved Hide resolved
packages/vite/src/node/preview.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 self-requested a review July 13, 2021 13:17
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

Seems there were some changes in main, could you also do a git rebase -i main?

packages/vite/src/node/preview.ts Outdated Show resolved Hide resolved
packages/vite/src/node/server/index.ts Outdated Show resolved Hide resolved
@ygj6
Copy link
Member Author

ygj6 commented Jul 14, 2021

Seems there were some changes in main, could you also do a git rebase -i main?

okay,already rebase

Shinigami92
Shinigami92 previously approved these changes Jul 14, 2021
packages/vite/src/node/preview.ts Outdated Show resolved Hide resolved
Shinigami92
Shinigami92 previously approved these changes Jul 14, 2021
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

@patak-js @ygj6 I will not have time for this today, but the requested changes should not stop us to merge it
It's just more nicer code refactoring requests

@patak-dev
Copy link
Member

We aren't in a rush, but yes, I don't see that this particular refactoring is needed. We can explore that in another PR if you want.

But after testing the PR, maybe I missed a discussion but what is the reason to avoid mirroring the API of dev for preview regarding strictPort? In dev, port is a hint to where to start looking for a free port, and users need to use strictPort if they want to avoid changing it. Shouldn't preview work in the same way?

@ygj6
Copy link
Member Author

ygj6 commented Jul 15, 2021

We aren't in a rush, but yes, I don't see that this particular refactoring is needed. We can explore that in another PR if you want.

But after testing the PR, maybe I missed a discussion but what is the reason to avoid mirroring the API of dev for preview regarding strictPort? In dev, port is a hint to where to start looking for a free port, and users need to use strictPort if they want to avoid changing it. Shouldn't preview work in the same way?

I also think that both dev server and preview should use strictPort, but this is somewhat ambiguous with the current behavior.
For example, dev server and preview set the port number in different ways :
preview is --port: https://vitejs.dev/guide/static-deploy.html#testing-the-app-locally
dev server is server.port: https://vitejs.dev/config/#server-port

In addition, server.strictPort is placed under server: https://vitejs.dev/config/#server-strictport
should it be adjusted to a shared option? is strictPort,and modify the description.

@patak-dev
Copy link
Member

You can also set the dev server port in the CLI https://github.com/vitejs/vite/blob/main/packages/vite/src/node/cli.ts#L67
And the --stricPort option is also present there.

We shouldn't use server.port and server.strictPort for preview as you said, this only affects the dev mode.

What I mean is that vite preview could work in the same way. We don't yet have a preview entry in the config, but at one point in the future we could add preview.port and preview.strictPort config options.
I think for this PR we could add a new --strictPort cli option in vite preview so both commands work in the same way. What do you think?

@ygj6
Copy link
Member Author

ygj6 commented Jul 17, 2021

The code is updated, please continue to help review, thank you.

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Looks good @ygj6. We'll discuss this PR with the team this week to seek approval. Thanks!

@antfu antfu merged commit 179a057 into vitejs:main Jul 19, 2021
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.

None yet

4 participants