feat(cli): add --target-host flag for dev servers not on localhost - #33
Open
bluzername wants to merge 1 commit into
Open
bluzername wants to merge 1 commit into
bluzername wants to merge 1 commit into
Conversation
…ocalhost Issue 0xnyn#21: user run local app on a subdomain like app.mylocalapp.com instead of localhost, and airship have no way to point at it. Server package already had a targetHost option inside, but CLI never expose it. This add a --target-host <hostname> flag, same validation as --host. It is used when we check the target is really listening (before it was always checking localhost even if user give different host), and it is passed to startServer so proxy forward to right place. Launch banner and --json output also show it now, so it not silently still say "localhost" when it is not true anymore. Test: new toServeOptions tests for target-host validation, and new launchBanner tests that check the proxying line names the custom host. Also test by hand with a real http.server on 127.0.0.1 with --target-host localhost, and with a host nothing listen on, to see the error message name the right host.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #21.
What this do
Add a
--target-host <hostname>flag so airship can proxy a dev serverthat is not on
localhost- for example a local subdomain likeapp.mylocalapp.com, which is what the issue ask for.The server package already had a
targetHostoption waiting inside(
packages/server/src/index.ts), it just was never wire up from theCLI. This PR:
args.ts), with same hostnamevalidation
--hostalready usetoServeOptionsan explicit
--targetand for the auto-detect path (before this itwas hardcoded to check
localhosteven when a different host wasgiven, so the "nothing is listening" error would be wrong)
startServer--jsonoutput, instead of alwaysprinting
localhostin the "proxying your dev server at..." lineeven when that is not true anymore
Not touching
--exec: when airship start the dev server itself it isalways a local child process, so that path keeps checking
localhoston purpose.
Test plan
apps/cli/src/commands/serve.test.ts(
toServeOptions - target host) for the validation.apps/cli/src/lib/banner.test.ts(launchBanner) thatcheck the proxying line names the custom host, and still say
localhostby default.pnpm typecheckandpnpm turbo run test --filter=@airshiplabs/cliboth green, 181 tests passed (176 before, +5 new).
./airship --target-host localhost --target 8199 --jsonagainst a realpython3 -m http.server 8199, got back"targetHost": "localhost"in the JSON and the right proxy URL.Also checked the error path with a host nothing is listening on -
message correctly names that host instead of localhost.
node scripts/sync-readme.mjs --checkpasses (README table updated,apps/cli/README.mdregenerated).I am not a collaborator here, just opening this for review, no rush.