Remove root parameter override in watch mode #925

tymokvo · 2024-06-26T02:28:33Z

Summary

Add optional watch flag for injecting a URL root into static site content for non-localhost browsers
Add case in if watch to replace root override with the passed URL root
Update injected websocket script to change request protocol based on window.location

Description

This adds the ability to serve the generated documentation site to browsers not running on localhost. For example, when working in a remote development environment like GitHub Codespaces or an ngrok endpoint.

nojaf

I believe these changes are too disruptive for any existing scenarios.
I think most people don't pass root as parameter and that should still work as everybody expects it.

build.fsx

nojaf · 2024-06-26T07:45:55Z

src/fsdocs-tool/BuildCommand.fs

-                    | _ -> None
-
-                r, userParameters
+        let userRoot =


This will clash later on with

FSharp.Formatting/src/fsdocs-tool/ProjectCracker.fs

Lines 611 to 614 in 2efb355

let root =

let projectUrl = projectInfoForDocs.PackageProjectUrl |> Option.map ensureTrailingSlash

defaultArg userRoot (defaultArg projectUrl ("/" + collectionName) |> ensureTrailingSlash)

Just following the instructions in the readme (dotnet build and src\fsdocs-tool\bin\Debug\net6.0\fsdocs.exe watch) will launch everything pointing to https://fsprojects.github.io/FSharp.Formatting and not localhost.

nojaf

I think this is heading in the right direction but I do have some remarks.

nojaf · 2024-06-28T12:31:05Z

src/fsdocs-tool/BuildCommand.fs

+    [<Option("contenthost", Required = false, HelpText = "URI root to inject in static content.")>]
+    member val contenthost = "" with get, set
+
+    override x.static_content_host_option =


This code violates some analyzers:

Run dotnet fsi build.fsx -- -p Verify for more info.

Thanks for the tip with -p Verify! That's helpful.

Though the output is quite verbose with results from the other projects. It also seems to create diffs in the tests/ directory for me. Not sure if that's intentional.

nojaf · 2024-06-28T12:36:52Z

src/fsdocs-tool/BuildCommand.fs

@@ -2115,3 +2119,13 @@ type WatchCommand() =

    [<Option("port", Required = false, Default = 8901, HelpText = "Port to serve content for http://localhost serving.")>]
    member val port = 8901 with get, set
+
+    [<Option("contenthost", Required = false, HelpText = "URI root to inject in static content.")>]
+    member val contenthost = "" with get, set


Please add documentation for this new setting.
I'm not quite convinced about the flag name and help text.
I also have some slight concerns that invalid input won't be displayed nicely to the end-users.
If you value for this is blah, Suave might trip over it.
It is also not quite clear if http:// or https:// needs to be included in this.

When you say:

Please add documentation for this new setting.

Do you mean just in the HelpText? Or is there somewhere else that I should be adding a more verbose description?

And good point about the invalid input. I added a call to System.Uri in the override to get its built-in parse errors. Do you think that is helpful enough?

The HelpText is a great start and I would like you to also add an entry in https://fsprojects.github.io/FSharp.Formatting/commandline.html (see https://github.com/fsprojects/FSharp.Formatting/blob/main/docs/commandline.md).

Could you take a look if we can show something friendlier than this?

Ah got it! And yes I will look into friendly-ifying the URI error.

nojaf · 2024-07-01T07:50:29Z

Running src\fsdocs-tool\bin\Debug\net6.0\fsdocs.exe watch --contenturlroot http://localhost:5001 --port 6001 is not quite working for me. I think we should not allow both settings at the same time.

launching browser window to open http://localhost:6001/
[09:48:03 INF] Smooth! Suave listener started in 8.653ms with binding 127.0.0.1:6001

but leads to

This also makes me wonder if we should go for a breaking change and introduce a --root flag instead? This would capture both the existing port and new contenturlroot flag.

Thoughts @kMutagene @nhirschey?

tymokvo · 2024-07-01T18:30:32Z

Yes, I noticed that those parameters could become out of sync. But isn't it possible that both --port and --contenturlroot may be desired?

E.g.

dev A is pairing with dev B on a branch
They want to see how their changes affect the docs
dev A starts the watch server bound to localhost:6001 (overriding the default of 8901)
dev A opens an ngrok tunnel from dev-a.ngrok.io to localhost:6001
dev B navigates to dev-a.ngrok.io and hits the suave server

In this case, the static content should all replace root with http://dev-a.ngrok.io/ and the suave server should still bind on 6001, correct?

nojaf · 2024-07-02T08:49:35Z

In this case, the static content should all replace root with http://dev-a.ngrok.io/ and the suave server should still bind on 6001, correct?

Hmm, yes, that is a plausible scenario. However, it does make me wonder if we should not bet more on making relative paths a thing?

tymokvo · 2024-07-02T19:10:43Z

Yes, that would also work, I think.

I am not very familiar with Suave, but I have used the rust and python abstractions for "just start an HTTP file server in this directory" before. I assume Suave has something similar or it's easy enough to create.

nojaf · 2024-07-03T05:48:50Z

I don't think this would be very Suave related. The bigger issues is that {{root}} is used all over the place and would need to transformed to the proper relative url.

nhirschey · 2024-07-03T10:14:04Z

Thoughts kMutagene nhirschey?

It seems complicated to document or explain contentroot, root, and port simultaneously. It's a bit of an edge case that anybody would use these together, and maybe a suboptimal edge case is ok, but still a bit of a hack.

If I understand the problem correctly, a single --root and then relative paths seems the cleanest way forward if it's possible. More work to get it right, but perhaps a better long-term foundation. I don't know why it's absolute rather than relative paths now.

kMutagene · 2024-07-05T06:37:39Z

Agreed with everything that has been said so far, but moving PRs to the 'More work to get it right, but perhaps a better long-term foundation' solution tends to stall PRs such as this - see my still-open PR of shame on this repo.

@tymokvo would you be up for escalating the scope of this PR that much? Otherwise i'd suggest publishing a 'good enough' solution only in preview versions, and another maintainer/contributor picking up from there in future PRs.

tymokvo · 2024-07-05T15:29:39Z

I would be happy to contribute a better long-term solution! But I was hoping to be able to get this merge-ready before the deadline I have at work now. So, I wouldn't be able to resume this for a couple weeks.

As long as you all are ok with this PR going stale for that amount of time, then I can revisit.

And, just so that I'm clear, the direction would be:

Support a single root flag to declare the URL root
Remove the existing override in the watch --root case
Update the path generation in web content to all use relative paths over absolute paths to {{root}}

...correct?

My main concern would be the risk of breaking changes since the scope would be so broad and would affect any project that depends on FSharp.Formatting.

nojaf · 2024-07-08T08:18:40Z

Hi @tymokvo, thanks for the offer. We have a call with the maintainers tomorrow, I'll bring it up over there.

nojaf · 2024-07-10T06:33:46Z

src/fsdocs-tool/BuildCommand.fs

@@ -1406,10 +1404,13 @@ type CoreBuildOptions(watch) =
        // Adjust the user substitutions for 'watch' mode root
        let userRoot, userParameters =
            if watch then
-                let userRoot = sprintf "http://localhost:%d/" this.port_option
+                let userRoot =


Would

let userRoot = match userParametersDict.TryGetValue(ParamKeys.root) with | true, root -> root | false, _ -> sprintf "http://localhost:%d/" this.port_option

work for you here?
I believe it can be an easy way to make

fsdocs watch --parameters root "/"

work and would be the least disruptive for now.

We don't feel good enough about the new setting.

I will test and let you know! Thanks for talking it through!

Sure thing, thanks for your patience!

tymokvo added 5 commits June 25, 2024 22:22

Remove root parameter override in watch mode

112b921

Remove comment

dfa48cc

Remove unnecessary reassignment

6cd58b9

Add pipelines for testing dotnet tool locally

22f4feb

Separate tool install and tool run

6e67326

tymokvo marked this pull request as ready for review June 26, 2024 03:44

tymokvo mentioned this pull request Jun 26, 2024

Enable watch server from arbitrary host #924

Open

nojaf requested changes Jun 26, 2024

View reviewed changes

tymokvo added 7 commits June 26, 2024 10:31

Revert build script changes

f9ac119

Revert watch override removal

aa0ea07

Add relative content option

0dcda55

Fix relativecontent option

61ed808

Set userRoot with relative_content_option

2df3a2c

Use browser window location in websocket watch script

f1fd7b3

Use string host option flag

6a313ab

nojaf requested changes Jun 28, 2024

View reviewed changes

tymokvo added 3 commits June 28, 2024 10:22

Update flag name and help text

d2e196c

Use System.Uri for parsing

4700b62

Remove /

86296aa

nojaf reviewed Jul 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove root parameter override in watch mode #925

Remove root parameter override in watch mode #925

tymokvo commented Jun 26, 2024 •

edited

Loading

nojaf left a comment

nojaf Jun 26, 2024

nojaf left a comment

nojaf Jun 28, 2024

tymokvo Jun 28, 2024

nojaf Jun 28, 2024

tymokvo Jun 28, 2024

nojaf Jul 1, 2024

nojaf Jul 1, 2024

tymokvo Jul 1, 2024

nojaf commented Jul 1, 2024

tymokvo commented Jul 1, 2024

nojaf commented Jul 2, 2024

tymokvo commented Jul 2, 2024

nojaf commented Jul 3, 2024

nhirschey commented Jul 3, 2024

kMutagene commented Jul 5, 2024

tymokvo commented Jul 5, 2024

nojaf commented Jul 8, 2024

nojaf Jul 10, 2024

tymokvo Jul 10, 2024

nojaf Jul 10, 2024

	let root =
	let projectUrl = projectInfoForDocs.PackageProjectUrl \|> Option.map ensureTrailingSlash

	defaultArg userRoot (defaultArg projectUrl ("/" + collectionName) \|> ensureTrailingSlash)

Remove root parameter override in watch mode #925

Are you sure you want to change the base?

Remove root parameter override in watch mode #925

Conversation

tymokvo commented Jun 26, 2024 • edited Loading

Summary

Description

nojaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf commented Jul 1, 2024

tymokvo commented Jul 1, 2024

nojaf commented Jul 2, 2024

tymokvo commented Jul 2, 2024

nojaf commented Jul 3, 2024

nhirschey commented Jul 3, 2024

kMutagene commented Jul 5, 2024

tymokvo commented Jul 5, 2024

nojaf commented Jul 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tymokvo commented Jun 26, 2024 •

edited

Loading