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

Add hot reload for the watch command (#627) #629

Merged
merged 6 commits into from
Jan 13, 2021
Merged

Add hot reload for the watch command (#627) #629

merged 6 commits into from
Jan 13, 2021

Conversation

kMutagene
Copy link
Contributor

@kMutagene kMutagene commented Jan 11, 2021

This is a WIP PR trying to implement hot reload for the fsdocs watch command as discussed in #627.

The server side part is done, and the cycle works when adding the following script to the _template.html files:

var wsUri = "ws://localhost:8901/websocket";
function init()
{
    websocket = new WebSocket(wsUri);
    console.log('connected')
    websocket.onclose = function(evt) { onClose(evt) };
}
function onClose(evt)
{
    console.log('closing');
    websocket.close();
    document.location.reload();
}
window.addEventListener("load", init, false);

But i am not sure what the best way to handle injecting this code is, as it has to be removed when building the actual docs, or the page will be loked in a constant reload cycle. My idea so far would be, when running the watch command, copy the template files, inject the code, and generate files based on that. This would also need an additional handling of updating these copied template files. I am not familiar enough with the code base yet to see what the best approach here is, and where best to intoduce these changes in the code, so feedback is welcome!


// handler for signalling to refresh the page to the client
let hotReloadHandler _ = Serve.signalHotReload <- true
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this hotReloadHandler get used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, that is indeed a leftover from a previous test, will remove

@dsyme
Copy link
Contributor

dsyme commented Jan 11, 2021

But i am not sure what the best way to handle injecting this code is, as it has to be removed when building the actual docs, or the page will be loked in a constant reload cycle. My idea so far would be, when running the watch command, copy the template files, inject the code, and generate files based on that. This would also need an additional handling of updating these copied template files. I am not familiar enough with the code base yet to see what the best approach here is, and where best to intoduce these changes in the code, so feedback is welcome!

I'm not sure of the best approach here - perhaps a template substitution that's only activated when building watch docs?

@kMutagene
Copy link
Contributor Author

I'm not sure of the best approach here - perhaps a template substitution that's only activated when building watch docs?

That worked well!

@kMutagene kMutagene marked this pull request as ready for review January 11, 2021 13:21
@kMutagene kMutagene changed the title [WIP] Add hot reload for the watch command (#627) Add hot reload for the watch command (#627) Jan 11, 2021
@dsyme
Copy link
Contributor

dsyme commented Jan 13, 2021

@kMutagene This looks ready?

@kMutagene
Copy link
Contributor Author

@dsyme yeah i was just not sure if i should merge my own PR or wait for a review ;)

@dsyme
Copy link
Contributor

dsyme commented Jan 13, 2021

@dsyme yeah i was just not sure if i should merge my own PR or wait for a review ;)

Feel free to ask for a review bia @ to myself and other maintainters. Then if you don't get one after a while you can self-merge :-)

docs/styling.md Outdated
@@ -104,6 +104,12 @@ If you write a new theme by CSS styling please contribute it back to FSharp.Form
You can do advanced styling by creating a new template. Add a file `docs/_template.html`, likely starting
with the existing default template.

to enable hot reload during development with `fsdocs watch` in a custom `_template.html` file, make sure to add the following line to your `<head>` tag:
Copy link
Contributor

Choose a reason for hiding this comment

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

Capital "T" at start of line.

Also there is a table of substitutions in content.md I believe, this should be added there

) }) )
)
}
|> Async.RunSynchronously
Copy link
Contributor

Choose a reason for hiding this comment

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

This has changed from Start to RunSynchronously. If it's RunSynchronously then no async is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, this is unnecessary. I was kind of thrown off by the 'fire and forget' nature of Async.Start (via this) and ditched it without second thought thinking i have to wait until the computation is finished. I missed that the hot reload trigger does not have to happen after the whole block but just after the last function call inside it, so i moved it inside the block now using the original Async.start version again.


regenerateSearchIndex())
}
|> Async.RunSynchronously
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise here....

@dsyme
Copy link
Contributor

dsyme commented Jan 13, 2021

On Start v. RunSynchronously - was this fixing a bug?

@kMutagene kMutagene requested a review from dsyme January 13, 2021 14:45
@dsyme dsyme merged commit af47b77 into fsprojects:master Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants