Skip to content

feat(config): hmr add disable port config#6624

Merged
patak-cat merged 3 commits intovitejs:mainfrom
jeoy:feat/hmr/optional-port
Mar 3, 2022
Merged

feat(config): hmr add disable port config#6624
patak-cat merged 3 commits intovitejs:mainfrom
jeoy:feat/hmr/optional-port

Conversation

@jeoy
Copy link
Contributor

@jeoy jeoy commented Jan 25, 2022 •
edited
Loading

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
View reviewed changes
@jeoy jeoy force-pushed the feat/hmr/optional-port branch from 0c73174 to c02efbe Compare January 26, 2022 05:13
@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

`

@jeoy jeoy force-pushed the feat/hmr/optional-port branch from bdee446 to c2b9bd1 Compare January 26, 2022 05:32
@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-cat
Copy link
Member

patak-cat commented Jan 26, 2022

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

@jeoy jeoy force-pushed the feat/hmr/optional-port branch from c2b9bd1 to c9dd9cb Compare January 26, 2022 12:45
@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.

@jeoy jeoy force-pushed the feat/hmr/optional-port branch from c9dd9cb to 5f19019 Compare January 26, 2022 12:48
bluwy
bluwy previously approved these changes Jan 26, 2022
View reviewed changes
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-cat
patak-cat previously approved these changes Jan 26, 2022
View reviewed changes
@patak-cat
Copy link
Member

patak-cat commented Jan 26, 2022

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

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

patak-cat commented Feb 12, 2022

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-cat and bluwy via e811f67 February 18, 2022 10:08
@jeoy jeoy force-pushed the feat/hmr/optional-port branch from e811f67 to 1f348c1 Compare February 21, 2022 09:05
@jeoy jeoy force-pushed the feat/hmr/optional-port branch from 1f348c1 to f663677 Compare February 21, 2022 09:07
@jeoy
Copy link
Contributor Author

jeoy commented Feb 21, 2022 •
edited
Loading

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.

bluwy
bluwy reviewed Feb 22, 2022
View reviewed changes
@jeoy @bluwy
Update docs/config/index.md
5d0ee67 
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@patak-cat patak-cat merged commit ce07a0a into vitejs:main Mar 3, 2022
@lukashass lukashass mentioned this pull request Mar 26, 2022
Merged
14 tasks
patak-cat added a commit that referenced this pull request Mar 30, 2022
@patak-cat
fix: revert #6624
3d9f878
@patak-cat patak-cat mentioned this pull request Mar 30, 2022
Merged
4 tasks
patak-cat added a commit that referenced this pull request Mar 30, 2022
@patak-cat
fix: revert #7463 and #6624 (#7522)
8efc6a6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patak-cat patak-cat patak-cat approved these changes

@bluwy bluwy bluwy approved these changes

+1 more reviewer

@Shinigami92 Shinigami92 Shinigami92 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

feat: hmr p2-nice-to-have Not breaking anything but nice to have (priority)

Projects

Team Board
Archived in project

Milestone

2.9

Development

Successfully merging this pull request may close these issues.

5 participants

@jeoy @bluwy @patak-cat @Shinigami92 @Niputi