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.

Sign up for GitHub

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

Jump to bottom

feat: added clientPort to HmrOptions #3578

Merged
merged 4 commits into from
May 29, 2021
Merged

feat: added clientPort to HmrOptions #3578

merged 4 commits into from
May 29, 2021

Conversation

johnnysprinkles
Copy link
Contributor

@johnnysprinkles johnnysprinkles commented May 28, 2021 •
edited
Loading

Description

There are times when it's useful to mismatch the HMR ports, meaning you run the websocket server on a different port than the client code looks for. The prime example is if you have SSL in front of your dev server using an SSL-terminating proxy, you'll also need to add SSL termination in front of your HMR websocket to avoid "mixed security" issues in the browser. But you can't have TLS and non TLS all on the same port.

So with this change, you'd be able let the websocket run on its default of 24678, but have the client code try to connect to ws://host:24679 for example, then use something like stunnel or any kind of SSL terminating proxy to route from 24679 => 24678.

Additional context

Nothing in particular, it's a minor change.

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

I was unable to get the tests to run, kept getting

portersi-macbookpro:hmr portersi$ pwd
/Users/portersi/projects/vite/packages/playground/hmr
portersi-macbookpro:hmr portersi$ yarn test
yarn run v1.22.10
error Command "test" not found. Did you mean "jest"?
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

If you help me out there I can run them.

  • 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.

@johnnysprinkles johnnysprinkles mentioned this pull request May 28, 2021
Closed
@Shinigami92 Shinigami92 added enhancement New feature or request feat: hmr labels May 28, 2021
@Shinigami92 Shinigami92 self-requested a review May 28, 2021 12:01
Shinigami92
Shinigami92 suggested changes May 28, 2021
View reviewed changes
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.

You may need to update the docs also https://github.com/vitejs/vite/blob/main/docs/config/index.md#serverhmr

@antfu
Copy link
Member

antfu commented May 28, 2021

Actually duplicated with #2921 #3115

This was referenced May 28, 2021
Closed
Closed
@Shinigami92
Copy link
Member

Actually duplicated with #2921 #3115

We decided that we want this clientPort-option solution 👍

@Shinigami92 Shinigami92 linked an issue May 28, 2021 that may be closed by this pull request
Vite HMR is unusable behind reverse proxies with random port numbers for client #3093
Closed
6 tasks
@johnnysprinkles
Copy link
Contributor Author

johnnysprinkles commented May 28, 2021 •
edited
Loading

Glad to hear! I feel bad about trampling over the previous related PRs but yes I'll update the doc, should be updated shortly. I'm new to open source Github contribution in general, I was going to try "git commit --amend" locally, I've always found a single commit per review is best in the past, but if a separate commit per feedback revision works best for the workflow I can do that as well.

@Shinigami92
Copy link
Member

Glad to hear! I feel bad about trampling over the previous related PRs but yes I'll update the doc, should be updated shortly. I'm new to open source Github contribution in general, I was going to try "git commit --amend" locally, I've always found a single commit per review is best in the past, but if a separate commit per feedback revision works best for the workflow I can do that as well.

I personally like a commit history, cause this way we can observe what was made
And due to we use squash-merges anyway, it's just one commit in the main branch later on 🤷
So nevertheless how many commits are in the PR, the main branch keeps clean

@johnnysprinkles
Copy link
Contributor Author

OK, added to the doc, as a separate commit. Not sure what the final commit message will be if you squash-merge, maybe just all the them concatenated? I'll find out when it happens.

@johnnysprinkles
Copy link
Contributor Author

Not sure if that second sentence is needed, the motivation for having the option, let me know what you think.

@johnnysprinkles
Copy link
Contributor Author

Woops, I wasn't doing the String() cast anymore, latest commit restores that.

@Shinigami92
Copy link
Member

The commit message will be the title of the PR

Shinigami92
Shinigami92 previously approved these changes May 28, 2021
View reviewed changes
docs/config/index.md Outdated Show resolved Hide resolved
@antfu antfu merged commit 7db69a3 into vitejs:main May 29, 2021
ygj6 pushed a commit to ygj6/vite that referenced this pull request Jun 1, 2021
@johnnysprinkles @ygj6
feat: added clientPort to HmrOptions (vitejs#3578)
f46455a
@fuzzykiller fuzzykiller mentioned this pull request Jun 2, 2021
Closed
6 tasks
@robots4life robots4life mentioned this pull request Apr 18, 2022
Open
@yitzolsberg yitzolsberg mentioned this pull request Sep 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antfu antfu antfu approved these changes

@Shinigami92 Shinigami92 Shinigami92 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request feat: hmr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Vite HMR is unusable behind reverse proxies with random port numbers for client
3 participants
@johnnysprinkles @antfu @Shinigami92