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(core): make webpack HMR always listen to current location #7951

Merged
merged 3 commits into from
Aug 18, 2022
Merged

fix(core): make webpack HMR always listen to current location #7951

merged 3 commits into from
Aug 18, 2022

Conversation

jeengbe
Copy link
Contributor

@jeengbe jeengbe commented Aug 12, 2022

See #7912 for explanation as to how these changes fix the problem. This does not have any side effects that I am aware of.

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Test Plan

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

Closes #7912

@facebook-github-bot
Copy link
Contributor

Hi @jeengbe!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@netlify
Copy link

netlify bot commented Aug 12, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 9c3497f
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62fddb423536ac0009a8ad34
😎 Deploy Preview https://deploy-preview-7951--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Aug 12, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟢 94 🟢 98 🟢 100 🟢 100 🟠 80 Report
/docs/installation 🟢 90 🟢 100 🟢 100 🟢 100 🟢 90 Report

@Josh-Cena Josh-Cena changed the title webpack HMR always listen to current location fix(core): make webpack HMR always listen to current location Aug 12, 2022
@Josh-Cena Josh-Cena added the pr: bug fix This PR fixes a bug in a past release. label Aug 12, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 12, 2022
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@slorber
Copy link
Collaborator

slorber commented Aug 12, 2022

This does not have any side effects that I am aware of.

Can you backup your claim with some reference to other projects using a similar setup?
If nobody uses this by default, there must be a reason 🤷‍♂️

I'd rather align to the behavior others are already using if we were to merge something not easy to double-check is correct for everyone.

For example CRA has env variables, but does not use such default:
https://github.com/facebook/create-react-app/blob/main/packages/react-scripts/config/webpackDevServer.config.js
facebook/create-react-app#7750

@jeengbe
Copy link
Contributor Author

jeengbe commented Aug 12, 2022

This does not have any side effects that I am aware of.

Can you backup your claim with some reference to other projects using a similar setup? If nobody uses this by default, there must be a reason 🤷‍♂️

I cannot, I just can't come up with a situation in which the new behaviour would be unwanted. As the websocket runs on the same domain + port the rest of the website is on, it always ought to be accessible on the same domain + port.
The only environment in which this would cause an issue is a reverse proxy that does not work with websockets, and the original port still exposed, and I do not see that happening in any dev envionment I can think of.

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Not familiar with the dev server architecture at all, so more of a question: does this change the experience with remote developing, like on GitHub codespaces? Right now we have to recommend --host 0.0.0.0.

@jeengbe
Copy link
Contributor Author

jeengbe commented Aug 18, 2022

It does change the experience in that it fixes HMR for when Docusaurus is launched on a different address/port is is being used with. --host 0.0.0.0 is still necessary for the server to work at all, but this pr eliminates issues with the socket connection (for HMR) being made to a different address/port (since setting to 0 makes it use the current location's)

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

The current configuration looks sensible to me (sounds like a no-op in most cases). I'll be willing to give it a try.

@slorber
Copy link
Collaborator

slorber commented Aug 18, 2022

Let's try this and we'll see if anyone complains. So far it seems to work for me.

For now I won't backport it in 2.x

@slorber slorber merged commit c706b23 into facebook:main Aug 18, 2022
@jeengbe jeengbe deleted the patch-1 branch August 18, 2022 17:53
@zevisert
Copy link

I for one would like to see this backported to 2.x! I brought up a question in discord that was likely solved by this PR. https://discord.com/channels/398180168688074762/867060369087922187/1012096405780844594

My use case is the same as #7912 -- it's that we want to run the docusaurus dev server on eg minikube locally. One of our requirements is to have TLS enabled in development for secure-context only JS features, like navigator.clipboard - and we plan on doing that with an ingress that terminates the TLS connection. We can get our SSL certs by proving a domain like local.example.com that points at 127.0.0.1.

Problem is that when deployed behind a reverse proxy ingress, the HMR websocket tries (and fails) to connect to the internal port like wss://local.example.com:3000/ws instead of wss://local.example.com/ws. I confirmed that @docusaurus/core@canary (0.0.0-5272 as of time of writing) solves this.

@slorber
Copy link
Collaborator

slorber commented Aug 25, 2022

@zevisert we could backport it but I'd prefer to have more user feedback first. For example, if nobody complains on any 3.0 upcoming alphas, it becomes safer to backport it

1 similar comment
@slorber
Copy link
Collaborator

slorber commented Aug 25, 2022

@zevisert we could backport it but I'd prefer to have more user feedback first. For example, if nobody complains on any 3.0 upcoming alphas, it becomes safer to backport it

@jeengbe
Copy link
Contributor Author

jeengbe commented Aug 28, 2022

Until the backport fix is out, remember that you can always put PR diffs in your patches folder and run patch-package.

https://patch-diff.githubusercontent.com/raw/facebook/docusaurus/pull/7951.diff

@masad-frost
Copy link

I noticed this never made it to 2.2.0, why?

@slorber
Copy link
Collaborator

slorber commented Dec 7, 2022

because we don't have a lot of feedback on potential side-effects due to v3.0 canaries not being widely, and I decided to not backport this:

#7951 (comment)

If you want to use it, wait for 3.0, use a canary release, or this temp workaround: #7912 (comment)

@slorber slorber mentioned this pull request Mar 10, 2023
7 tasks
@mi-na-bot
Copy link

I am trying to use a 2.4 project in GitHub Codespaces, which sets up an HTTP proxy for port forwards, and this issue is a major disappointment/pain. One zero-side-effect option would be to allow the user to enable this with the CLI. Having the dev server listen on the same host/port as the original connection and the client component look for it there is exactly what Next.js has been doing for quite some time with no ill effect. IMO, the current configuration only works because of a side effect.

@mi-na-bot
Copy link

Applying the change from this PR in my node_modules directory fixes the issue in GitHub Codespaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Live reload not working with reverse proxy
7 participants