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: allow different host through CLI flag #1475

Merged
merged 7 commits into from
May 20, 2019
Merged

feat: allow different host through CLI flag #1475

merged 7 commits into from
May 20, 2019

Conversation

palmer-cl
Copy link
Contributor

Motivation

This would allow folks running Docusaurus in a container to access the site on localhost.

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

From: #920 which appears closed.

  • Install a fresh Docuaurus in the host environment.
  • Run yarn start in the host environment.
  • Ensure the site is served at http://localhost:3000 as usual.
  • Containerize the application (get it installed in a node based container)
  • Run yarn start within the container.
  • Confirm the site is not accessible at http://localhost:3000
  • Stop the webserver (live reload).
  • Apply the change from this merge request commit.
  • Run yarn start within the container.
  • Confirm the site is accessible at http://localhost:3000

Note: I have tested this both locally and running inside a Docker container.

Related PRs

#920

Fixes #919
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@palmer-cl palmer-cl requested a review from endiliey as a code owner May 17, 2019 00:55
@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up 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 the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented May 17, 2019 •
edited
Loading

Deploy preview for docusaurus-2 ready!

Built with commit b3ff653

https://deploy-preview-1475--docusaurus-2.netlify.com

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented May 17, 2019 •
edited
Loading

Deploy preview for docusaurus-preview ready!

Built with commit b3ff653

https://deploy-preview-1475--docusaurus-preview.netlify.com

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 17, 2019
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

yangshun
yangshun suggested changes May 17, 2019
View reviewed changes
Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I don't think the change works for non-Windows users currently. I think we should only change it to 0.0.0.0 when using a container's platform or instead allow the host to be specified as a CLI option if containers aren't tied to a particular platform.

packages/docusaurus-1.x/lib/server/__tests__/liveReloadServer.test.js Outdated Show resolved Hide resolved
packages/docusaurus-1.x/lib/server/liveReloadServer.js Outdated Show resolved Hide resolved
@palmer-cl
Copy link
Contributor Author

@yangshun Great feedback. I can make some changes.

@endiliey endiliey self-assigned this May 17, 2019
@palmer-cl
Copy link
Contributor Author

@endiliey Let me know what you think of this implementation.

endiliey
endiliey suggested changes May 19, 2019
View reviewed changes
Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

There are some part still using localhost

Example:

https://github.com/facebook/Docusaurus/blob/c9c13209c228ba27c8b5a634fba96f79fcdf8025/packages/docusaurus-1.x/lib/server/server.js#L358-L377

@endiliey endiliey changed the title feat(core): use 0.0.0.0 for livereload address feat: allow different host through CLI flag May 19, 2019
endiliey
endiliey reviewed May 20, 2019
View reviewed changes
website-1.x/versioned_docs/version-1.8.0/api-commands.md Outdated
@@ -147,6 +147,7 @@ This command will build the static website, apply translations if necessary, and
| Options | Default | Description |
| ----------------- | ------- | ------------------------------------------------------------------------------------------------------------------------------------ |
| `--port <number>` | `3000` | The website will be served from port 3000 by default, but if the port is taken up, Docusaurus will attempt to find an available one. |
|`--host <number>`|`localhost`|Use a custom host with localhost as the default.|
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it host ?

Copy link
Contributor Author

@palmer-cl palmer-cl May 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Per discussion: https://github.com/facebook/Docusaurus/pull/1475/files/326d299e5baeaee3f7dc6791eb96bf05de82767a#discussion_r284958920

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops my bad. I mean why host <number>.

GitHub swallowed the <number>

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha. All good. Ya, I figured number as in 127.0.0.1 but maybe 'value' is better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Follow v2

https://github.com/facebook/Docusaurus/blob/382b88bb49cc13adb920dbb0b901afa44921b041/packages/docusaurus/bin/docusaurus.js#L82

endiliey
endiliey approved these changes May 20, 2019
View reviewed changes
Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

I've patched it a bit. Anyway, thanks a lot 😉

🚀 Ship it ....

shipit

@endiliey endiliey merged commit 0568ad4 into facebook:master May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yangshun yangshun yangshun requested changes

@endiliey endiliey endiliey approved these changes

Assignees

@endiliey endiliey

Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use 0.0.0.0 for livereload address
5 participants
@palmer-cl @facebook-github-bot @docusaurus-bot @yangshun @endiliey