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

Remove root parameter override in watch mode #925

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Remove root parameter override in watch mode #925

Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
112b921
Remove root parameter override in watch mode
tymokvo Jun 26, 2024
dfa48cc
Remove comment
tymokvo Jun 26, 2024
6cd58b9
Remove unnecessary reassignment
tymokvo Jun 26, 2024
22f4feb
Add pipelines for testing dotnet tool locally
tymokvo Jun 26, 2024
6e67326
Separate tool install and tool run
tymokvo Jun 26, 2024
f9ac119
Revert build script changes
tymokvo Jun 26, 2024
aa0ea07
Revert watch override removal
tymokvo Jun 26, 2024
0dcda55
Add relative content option
tymokvo Jun 26, 2024
61ed808
Fix relativecontent option
tymokvo Jun 26, 2024
2df3a2c
Set userRoot with relative_content_option
tymokvo Jun 26, 2024
f1fd7b3
Use browser window location in websocket watch script
tymokvo Jun 26, 2024
6a313ab
Use string host option flag
tymokvo Jun 26, 2024
d2e196c
Update flag name and help text
tymokvo Jun 28, 2024
4700b62
Use System.Uri for parsing
tymokvo Jun 28, 2024
86296aa
Remove /
tymokvo Jun 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 23 additions & 9 deletions src/fsdocs-tool/BuildCommand.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,11 +792,11 @@ module Serve =
let refreshEvent = FSharp.Control.Event<string>()

/// generate the script to inject into html to enable hot reload during development
let generateWatchScript (port: int) =
let tag =
"""
let generateWatchScript =
"""
<script type="text/javascript">
var wsUri = "ws://localhost:{{PORT}}/websocket";
var loc = window.location.valueOf();
var wsUri = `${loc.protocol.replace("http", "ws")}//${loc.host}/websocket`;
function init()
{
websocket = new WebSocket(wsUri);
Expand All @@ -823,8 +823,6 @@ module Serve =
</script>
"""

tag.Replace("{{PORT}}", string<int> port)

let connectedClients = ConcurrentDictionary<WebSocket, unit>()

let socketHandler (webSocket: WebSocket) (context: HttpContext) =
Expand Down Expand Up @@ -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 =
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure thing, thanks for your patience!

match this.static_content_host_option with
| Some s -> s
| _ -> sprintf "http://localhost:%d/" this.port_option

if userParametersDict.ContainsKey(ParamKeys.root) then
printfn "ignoring user-specified root since in watch mode, root = %s" userRoot
printfn "ignoring user-specified root since in watch mode, root = '%s'" userRoot

let userParameters =
[ ParamKeys.root, userRoot ]
Expand Down Expand Up @@ -1667,7 +1668,7 @@ type CoreBuildOptions(watch) =
let getLatestWatchScript () =
if watch then
// if running in watch mode, inject hot reload script
[ ParamKeys.``fsdocs-watch-script``, Serve.generateWatchScript this.port_option ]
[ ParamKeys.``fsdocs-watch-script``, Serve.generateWatchScript ]
else
// otherwise, inject empty replacement string
[ ParamKeys.``fsdocs-watch-script``, "" ]
Expand Down Expand Up @@ -2088,6 +2089,9 @@ type CoreBuildOptions(watch) =
abstract port_option: int
default x.port_option = 0

abstract static_content_host_option: string option
default x.static_content_host_option = None

[<Verb("build", HelpText = "build the documentation for a solution based on content and defaults")>]
type BuildCommand() =
inherit CoreBuildOptions(false)
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

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

Copy link
Author

Choose a reason for hiding this comment

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

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


override x.static_content_host_option =
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code violates some analyzers:

image

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

match x.contenthost with
| "" -> None
| s ->
if not (s.EndsWith("/")) then $"%s{s}/" else s
|> Some
Loading