Add `--host` option to `docc preview` #713

tomstuart · 2023-09-25T07:23:14Z

Summary

I installed Swift-DocC on a remote server and wanted to run docc preview to preview my docs with my local browser, but it didn’t work because the preview server binds to the loopback interface (localhost) without any option to receive connections from other hosts.

This PR adds a --host option so that the preview server can be bound to an address on a real network interface (e.g. the machine’s IP address, or just 0.0.0.0 to bind to all IPv4 interfaces) to allow other hosts to connect.

Testing

Steps:

Run swift run docc preview as described in CONTRIBUTING.md
On another machine, try visiting http://<server-address>:8080/documentation/swiftdocc in a browser, which will fail to connect
Stop the preview server and run swift run docc preview again, this time providing the --host 0.0.0.0 option
Visit http://<server-address>:8080/documentation/swiftdocc again and see it work

Checklist

Added tests
Ran the ./bin/test script and it succeeded
Updated documentation if necessary

Although I’ve updated the existing tests to support this change, I can’t see any tests which actually exercise the network behaviour of docc preview, so I didn’t feel comfortable introducing one here.

tomstuart · 2023-09-25T11:53:24Z

Once this is merged I intend to open another PR to rename the enumeration cases from localhost and socket to something more descriptive, e.g. ip and unix, to make it clearer that the IP server is no longer necessarily bound to the loopback interface.

franklinsch · 2023-09-25T11:56:11Z

Tests/SwiftDocCUtilitiesTests/PreviewServer/PreviewServerTests.swift

@@ -201,7 +201,7 @@ class PreviewServerTests {
    }

    func testPreviewServerBindDescription() {
-        let localhostBind = PreviewServer.Bind.localhost(port: 1234)
+        let localhostBind = PreviewServer.Bind.localhost(host: "localhost", port: 1234)


Should we also test that by default, "localhost" is the host?

It’s the default in the sense that the --host option (in PreviewOptions) now has the value localhost by default, but PreviewServer itself has no default host — an explicit value is always required. Are you suggesting we add a PreviewOptions test to check that --host gets the right default, or something different? There’s not currently a PreviewOptionsTests.

Yes, it would be good to test that --host gets the right default. The best place for that would be in PreviewSubcommandTests, it looks like.

Sources/SwiftDocCUtilities/ArgumentParsing/Options/PreviewOptions.swift

d-ronnqvist · 2023-09-25T16:02:36Z

Sources/SwiftDocCUtilities/Action/Actions/PreviewAction.swift

@@ -84,6 +86,7 @@ public final class PreviewAction: Action, RecreatingContext {
    ///     is performed.
    /// - Throws: If an error is encountered while initializing the documentation context.
    public init(
+        host: String,


Since this is a public initializer, providing a default value here would avoid a source breaking change for anyone who imports SourceDocCUtilities.

Sources/SwiftDocCUtilities/ArgumentParsing/Options/PreviewOptions.swift

`ServerBootstrap.bind` needs to know which host name to bind a socket to, but we were hardcoding it to `localhost` without providing a way for a different host to be provided. By adding a `host:` element to `PreviewServer.Bind.localhost`, we require the caller of `PreviewServer.init` to be specific about which host they want.

If there’s a problem binding to a local port, the specific host name might be relevant.

Now that `PreviewServer` requires an explicit host name, we should require one here too instead of hardcoding `localhost`.

This retains the `localhost` default host name for the preview server, but allows the user to override it by providing `--host`. We’re only supporting the long `--host` option because `-h` already means `--help`.

Because `localhost` only allows connections on the loopback interface, anybody who’s trying to run `docc preview` on a remote server and connect to it with their local browser will need to use `--host 0.0.0.0` or similar.

Kyle-Ye

I'm generally -0.5 for this PR.

IMO, Preview is actually a little out of scope of Swift-DocC. So we only provide basic support for it(Just my opinion).
If people want advanced feature for the preview(eg. check the preview on other network device), they can start a http server manually. It's very easy for almost every OS and in all language. We should not add complex here on DocC side.
The current API design of PreviewAction is public. So we must preserve the old API to maintain source compatibility. It's actually a bit complex.

If other DocC folks agrees the idea of the PR. You can have a look at #254 for reference to fix the compatibility issue.

Kyle-Ye · 2023-10-04T17:07:23Z

Sources/SwiftDocCUtilities/Action/Actions/PreviewAction.swift

@@ -108,13 +112,14 @@ public final class PreviewAction: Action, RecreatingContext {
    @available(*, deprecated, message: "TLS support has been removed.")
    public convenience init(
        tlsCertificateKey: URL?, tlsCertificateChain: URL?, serverUsername: String?,
-        serverPassword: String?, port: Int,
+        serverPassword: String?, host: String, port: Int,


This initializer is already deprecated, we should not change the signature

Kyle-Ye · 2023-10-04T17:08:26Z

Sources/SwiftDocCUtilities/Action/Actions/PreviewAction.swift

@@ -84,6 +86,7 @@ public final class PreviewAction: Action, RecreatingContext {
    ///     is performed.
    /// - Throws: If an error is encountered while initializing the documentation context.
    public init(
+        host: String,


duplicate the original init and add host parameter.

mark the original init as deprecate.

Like I commented here I think it's sufficient to provide a default value for this added parameter to remain source compatible. It won't be ABI compatible but we don't require that.

Yeah. Ethan's PR #254 is removing parameters. So he deprecated the old and add a new one.
For this PR, this is just an addition. If we only cares about source compatible, it is indeed enough to provide a default value.

tomstuart · 2023-10-04T17:34:41Z

If people want advanced feature for the preview(eg. check the preview on other network device), they can start a http server manually. It's very easy for almost every OS and in all language. We should not add complex here on DocC side.

I don’t disagree, but (for whatever reason) we already provide --port as a way to override the default value for one of ServerBootstrap.bind’s arguments; my intention here is to provide --host as a way to override the other one.

Kyle-Ye · 2023-10-04T17:43:48Z

but (for whatever reason) we already provide --port as a way to override the default value for one of ServerBootstrap.bind’s arguments;

The --port support was added when swift-docc is open sourced for the first commit. Maybe @d-ronnqvist know the context here.
The port used to default to 8000. And it changed to 8080 on #373

my intention here is to provide --host as a way to override the other one.

Got it. Make sense for me.

d-ronnqvist · 2023-10-05T21:19:15Z

but (for whatever reason) we already provide --port as a way to override the default value for one of ServerBootstrap.bind’s arguments;

The --port support was added when swift-docc is open sourced for the first commit. Maybe @d-ronnqvist know the context here. The port used to default to 8000. And it changed to 8080 on #373

my intention here is to provide --host as a way to override the other one.

Got it. Make sense for me.

I feel that it's fine to add a --host option to the preview command when we already have a --port option.

It's been several years so I don't remember any particular context around when we added the --port option. It may have "always" been there.

tomstuart force-pushed the preview-server-host-option branch from 71ff7db to 55cb318 Compare September 25, 2023 11:54

franklinsch reviewed Sep 25, 2023

View reviewed changes

Sources/SwiftDocCUtilities/ArgumentParsing/Options/PreviewOptions.swift Show resolved Hide resolved

d-ronnqvist reviewed Sep 25, 2023

View reviewed changes

tomstuart force-pushed the preview-server-host-option branch 2 times, most recently from 780cc18 to a500b6b Compare September 27, 2023 14:46

tomstuart added 5 commits September 27, 2023 16:16

Include host name in PreviewServer error messages

2a1539a

If there’s a problem binding to a local port, the specific host name might be relevant.

Add host: parameter to PreviewAction.init

f8be787

Now that `PreviewServer` requires an explicit host name, we should require one here too instead of hardcoding `localhost`.

Add --host option to PreviewOptions

e4cc3c1

This retains the `localhost` default host name for the preview server, but allows the user to override it by providing `--host`. We’re only supporting the long `--host` option because `-h` already means `--help`.

Add --host advice to docc preview instructions

845db03

Because `localhost` only allows connections on the loopback interface, anybody who’s trying to run `docc preview` on a remote server and connect to it with their local browser will need to use `--host 0.0.0.0` or similar.

tomstuart force-pushed the preview-server-host-option branch from a500b6b to 845db03 Compare September 27, 2023 15:43

franklinsch requested a review from d-ronnqvist September 29, 2023 11:02

Kyle-Ye requested changes Oct 4, 2023

View reviewed changes

d-ronnqvist added the source breaking DocC's public API isn't source compatible with earlier versions label Nov 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--host` option to `docc preview` #713

Add `--host` option to `docc preview` #713

tomstuart commented Sep 25, 2023

tomstuart commented Sep 25, 2023

franklinsch Sep 25, 2023

tomstuart Sep 25, 2023

franklinsch Sep 25, 2023

d-ronnqvist Sep 25, 2023

Kyle-Ye left a comment

Kyle-Ye Oct 4, 2023

Kyle-Ye Oct 4, 2023

d-ronnqvist Oct 4, 2023 •

edited

Kyle-Ye Oct 4, 2023

tomstuart commented Oct 4, 2023

Kyle-Ye commented Oct 4, 2023 •

edited

d-ronnqvist commented Oct 5, 2023

Add --host option to docc preview #713

Are you sure you want to change the base?

Add --host option to docc preview #713

Conversation

tomstuart commented Sep 25, 2023

Summary

Testing

Checklist

tomstuart commented Sep 25, 2023

franklinsch Sep 25, 2023

Choose a reason for hiding this comment

tomstuart Sep 25, 2023

Choose a reason for hiding this comment

franklinsch Sep 25, 2023

Choose a reason for hiding this comment

d-ronnqvist Sep 25, 2023

Choose a reason for hiding this comment

Kyle-Ye left a comment

Choose a reason for hiding this comment

Kyle-Ye Oct 4, 2023

Choose a reason for hiding this comment

Kyle-Ye Oct 4, 2023

Choose a reason for hiding this comment

d-ronnqvist Oct 4, 2023 • edited

Choose a reason for hiding this comment

Kyle-Ye Oct 4, 2023

Choose a reason for hiding this comment

tomstuart commented Oct 4, 2023

Kyle-Ye commented Oct 4, 2023 • edited

d-ronnqvist commented Oct 5, 2023

Add `--host` option to `docc preview` #713

Add `--host` option to `docc preview` #713

d-ronnqvist Oct 4, 2023 •

edited

Kyle-Ye commented Oct 4, 2023 •

edited