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(config): hmr add disable port config #6624

Merged
merged 3 commits into from Mar 3, 2022

Conversation

jeoy
Copy link
Contributor

@jeoy jeoy commented Jan 25, 2022

Description

HMR connecting failed when I have to config a hmr port or using default port.

Unexpected socket host: www.mydomain.com:3000
Expected socket host: www.mydomain.com

image

Using default port cannot solve this problem completely when I need both http(80) and https(443) domain to access.

Adding a custom hmr host config is not a perfect solution, cause I have to config different hmr host for every project.

So, I was wondering if we can add disable port config for this situation?

Solution 1:
Change port: number -> port: number | false.

Solution 2:
add new config: disablePort: boolean (default false)

below is changes of Solution 1.

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.
  • [ x 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 previously approved these changes Jan 25, 2022
@jeoy
Copy link
Contributor Author

jeoy commented Jan 26, 2022

CI failed caused by this line

code.replace should be code.replaceAll for an injected global var used more than once

`

@bluwy
Copy link
Member

bluwy commented Jan 26, 2022

below is changes of Solution 1.

Looks like the changes now are Solution 2 though. I think Solution 1 seems nicer to avoid another option.

@jeoy
Copy link
Contributor Author

jeoy commented Jan 26, 2022

yeah... but solution 1 make type of port a bit weird, don't you think?
Another option seems to in conformity with the SRP.

@jeoy
Copy link
Contributor Author

jeoy commented Jan 26, 2022

below is changes of Solution 1.

Looks like the changes now are Solution 2 though. I think Solution 1 seems nicer to avoid another option.

Hi @bluwy , please review again when you convenient, thanks!

@patak-dev
Copy link
Member

I also think that solution 1, port: number -> port: number | false is better here

@jeoy
Copy link
Contributor Author

jeoy commented Jan 26, 2022

I also think that solution 1, port: number -> port: number | false is better here

Sure, now is solution 1 already.

bluwy
bluwy previously approved these changes Jan 26, 2022
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for making the change.

patak-dev
patak-dev previously approved these changes Jan 26, 2022
@patak-dev
Copy link
Member

We'll discuss it in the next meeting to validate the API change. Thanks @jeoy!

@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Jan 26, 2022
@patak-dev patak-dev added this to the 2.9 milestone Feb 12, 2022
@patak-dev
Copy link
Member

We are good to merge this PR, let's do it in the 2.9 beta (we should start it soon)

@bluwy
Copy link
Member

bluwy commented Feb 13, 2022

I think we should update the types in the docs too.

@jeoy jeoy dismissed stale reviews from patak-dev and bluwy via e811f67 February 18, 2022 10:08
@jeoy
Copy link
Contributor Author

jeoy commented Feb 21, 2022

rebase main branch and resolve conflict

@bluwy
Copy link
Member

bluwy commented Feb 21, 2022

This LGTM with one more update to the docs at https://vitejs.dev/config/#server-hmr

@jeoy
Copy link
Contributor Author

jeoy commented Feb 22, 2022

This LGTM with one more update to the docs at https://vitejs.dev/config/#server-hmr

Docs updated.

docs/config/index.md Outdated Show resolved Hide resolved
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@patak-dev patak-dev merged commit ce07a0a into vitejs:main Mar 3, 2022
patak-dev added a commit that referenced this pull request Mar 30, 2022
@patak-dev patak-dev mentioned this pull request Mar 30, 2022
4 tasks
patak-dev added a commit that referenced this pull request Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: hmr p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants