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: expose server on local network with new --host flag #2760

Merged
merged 27 commits into from
Mar 11, 2022

Conversation

bholmesdev
Copy link
Contributor

@bholmesdev bholmesdev commented Mar 10, 2022

Changes

  • Introduce new --host flag to mirror Vite's config option
    • When --host is absent, the network URL is not exposed on your network
    • When --host is present, the network URL is exposed at a default URL
    • When --host [custom host] is present, the network URL is exposed at [custom host]
    • (deprecated) When --hostname [custom host] is present, the network URL is also exposed at [custom host]
  • Deprecate --hostname while maintaining functionality

Testing

  • Added unit tests for current and legacy flags
  • Added unit tests for preview command alongside dev

Docs

Updated configuration and CLI references on docs.
PR here!

@changeset-bot
Copy link

changeset-bot bot commented Mar 10, 2022

🦋 Changeset detected

Latest commit: fdf08de

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
astro Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) test labels Mar 10, 2022
* @type {string}
* @default `'localhost'`
* @default `localhost`
* @deprecated Use --host instead
Copy link
Member

Choose a reason for hiding this comment

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

how should we handle this in the docgen script?

I think my suggestion would be to ignoring it completely, and add a note in the host @description body that it used to be called hostname.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made that refactor. Waiting for the docgen updates to merge, but will verify my > blockquote looks right once those are in 👍

@@ -38,6 +38,7 @@ function printHelp() {
title('Flags');
table(
[
['--host [optional host address]', 'Expose server on network'],
Copy link
Member

Choose a reason for hiding this comment

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

nit: this feels a little verbose for just a summary. What about just '--host [address]' or '--host [ip]'? imo advanced details can be left to the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair! Just wanted to clarify that it's optional, so users know --host could be a boolean flag. Maybe --host [optional IP] is a good compromise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

☝️ Went ahead with that refactor. Lmk if you like it!

const site = config.buildOptions.site ? new URL(config.buildOptions.site) : undefined;
info(
options.logging,
null,
msg.devStart({ startupTime: performance.now() - devStart, port: address.port, localAddress, networkAddress: address.address, site, https: !!viteUserConfig.server?.https })
msg.devStart({ startupTime: performance.now() - devStart, port: address.port, localAddress, isNetworkExposed, site, https: !!viteUserConfig.server?.https })
Copy link
Member

Choose a reason for hiding this comment

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

I thought that --host 1.2.3.4 would work. How does devStart know to show the correct http://1.2.3.4 URL in the console log if we don't share the host string? Is os.networkInterfaces() that smart?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, os.networkInterfaces really does cover us! This is taken directly from Vite's CLI.
For instance, passing my device IP will only expose the server on my network (not localhost or 0.0.0.0):

Screen Shot 2022-03-11 at 9 00 53 AM

Copy link
Member

Choose a reason for hiding this comment

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

cool! TIL

Copy link
Member

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

Looking good! Left a few comments

@github-actions github-actions bot added the pkg: example Related to an example package (scope) label Mar 11, 2022
@bholmesdev bholmesdev force-pushed the feat/expose-local-network-on-host-flag branch 3 times, most recently from 777554f to e2aef14 Compare March 11, 2022 17:11
@bholmesdev bholmesdev force-pushed the feat/expose-local-network-on-host-flag branch from e2aef14 to 6bea3bb Compare March 11, 2022 18:03
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Looks good other than a question around passing --host 127.0.0.1 explicitly

examples/minimal/astro.config.mjs Outdated Show resolved Hide resolved
packages/astro/src/core/dev/util.ts Show resolved Hide resolved
@natemoo-re natemoo-re added this to the 0.24 milestone Mar 11, 2022
@github-actions github-actions bot removed the pkg: example Related to an example package (scope) label Mar 11, 2022
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

LGTM once tests are passing!

Comment on lines +9 to +10
import type { AddressInfo } from 'net';
import type { AstroConfig } from '../@types/astro';
Copy link
Member

Choose a reason for hiding this comment

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

Super small nit: we always do import type at the top of the file.

No need to fix, just FYI.

@bholmesdev bholmesdev force-pushed the feat/expose-local-network-on-host-flag branch from cce64e8 to fdf08de Compare March 11, 2022 22:04
@natemoo-re
Copy link
Member

natemoo-re commented Mar 11, 2022

Image

@bholmesdev bholmesdev merged commit 77b9c95 into main Mar 11, 2022
@bholmesdev bholmesdev deleted the feat/expose-local-network-on-host-flag branch March 11, 2022 22:25
@github-actions github-actions bot mentioned this pull request Mar 11, 2022
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
…2760)

* feat: update config to support bool --hostname

* fix: show localhost for --hostname=true

* feat: address logging feature parity w/ Vite

* chore: update type docs

* refactor: extract local, network prefs to variable

* feat: add --host to --help output

* feat: deprecate --hostname, add --host

* feat: add --host tests

* feat: update preview to support new flags

* fix: show --host in dev server log

* feat: update config tests for --host flag

* chore: test lint

* chore: update lock with new fixture

* chore: add changeset

* refactor: add more details to JSdocs

* fix: update path tests

* feat: only expose when --host is not local

* fix: make flag --help less verbose

* fix: address @types comments

* fix: lint

* chore: remove unused import

* fix: use host flag for config test

* fix: ensure local logs come before network

* refactor: switch up that network logging one last time!

* feat: update unit tests

* chore: remove debugging block

* fix: only parse network logs if network is present
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants