search

dotnet / docfx

Static site generator for .NET API documentation.
https://dotnet.github.io/docfx/
MIT License
3.94k stars 839 forks source link

[docfx] adding --watch option to build command #10010

Open rmannibucau opened 2 weeks ago

rmannibucau commented 2 weeks ago

Follow-up of my tests (https://github.com/dotnet/docfx/pull/9986) and https://github.com/dotnet/docfx/discussions/9992

Not 100% sure of the following so happy to get a feedback on all these points:

  1. didn't see much tests of the build/serve mode so does it need a test (regarding project compliance)?
  2. I'm not very clear why default command is not build command but a kind of fork not doing exactly the same thing so maybe I didn't patch the right location
  3. the glob to pattern logic is very simple to start (can always be enhanced later), would be happy to get an ack it is sufficient to start or not from your experience (I'm very new to docfx and don't know all the subtleties yet)
  4. I opened docfx.json model, hope it is ok (I assumed thanks to #9986 it was maybe an option but not 100% sure)
codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 13.74046% with 113 lines in your changes missing coverage. Please review.

Project coverage is 78.48%. Comparing base (fe673ec) to head (74b6c34). Report is 215 commits behind head on main.

Files Patch % Lines
src/docfx/Models/WatchCommand.cs 0.00% 107 Missing :warning:
src/docfx/Models/DefaultBuildCommandOptions.cs 85.00% 3 Missing :warning:
src/docfx/Models/WatchCommandOptions.cs 0.00% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #10010 +/- ## ========================================== + Coverage 74.31% 78.48% +4.16% ========================================== Files 536 543 +7 Lines 23189 23590 +401 Branches 4056 4078 +22 ========================================== + Hits 17234 18514 +1280 + Misses 4853 3934 -919 - Partials 1102 1142 +40 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

filzrev commented 2 weeks ago

In my personal opinion. This feature should be implemented as separated docfx watch command (#2297). And existing docfx and docfx build commands are leave as is.

Because docfx watch feature requires different set of options/configs. And implementation can be simpler for future command extensibilities.

For example.

rmannibucau commented 2 weeks ago

Because docfx watch feature requires different set of options/configs. And implementation can be simpler for future command extensibilities.

Hmm, it has serve capabilities so adding watch which is a serve companion or concurrent is not shocking to me. To be concrete if we do a watch command and follow the reasoning it means watch will not serve right? But then we need yet another watch+serve command so not sure it is great. I'm coming from a projects where commands are build (plain build no serve or watch command) and serve or watch which both handle both features (optionally with live reload support) so I'm surely biased but my experience with docfx is I have no idea how serve can work since to work on the doc I needed to setup inotify and a plain http-server was replacing serve command quite quickly so maybe I'm missing some usage there - happy to learn if so.

For example.

Watch _site contents and reload browser window (LiveReload/BrowserRefresh) similar to dotnet watch command.

Kind of requires serve to be there as well no?

Support Ctrl+R hotkey to force rebuild. similar to dotnet watch command. Support pseudo incremental build (Build only changed document)

+1 (it is the last else block where watcher is started without serve feature) and I'm happy to do a watch only feature extracted from from this PR but we still need an all in one command, where would it fit if not build one which is already an all in one? Also what about the default command convergence with build or should it be used as all in one command?

filzrev commented 2 weeks ago

Thank you for confirming my PR comments

I'm expecting following behaviors by docfx watch command. (If it's added by this PR)

When docfx watch command executed. By default, following processes are executed on separated HostedService thread.

  1. Serve _site directory content with ASP.NET. (And inject livereload-js scripts to HTML file)
  2. Watch changes of _site directory files. When changes are detected. Send LiveReload command to browser.
  3. Watch changes of docs files. And run build when files are modified.
  4. Watch keyboard input. And When Ctrl+R key pressed. Request docfx build.

And by adding watch section to docfx.json. It can change behaviors.

If basic docfx watch command infrastructure is added by this PR. I'm also willing creating PR that relating to this features. (Ctrl+R hotkey support and LiveReload supports)

rmannibucau commented 2 weeks ago

@filzrev I'm in the process of moving build to watch command (restoring old build command), hope I got it right

rmannibucau commented 1 week ago

Anything I can do to help getting this on board?

filzrev commented 1 week ago

Anything I can do to help getting this on board?

To supporting docfx watch command. I thought It need to add following statement to Program.cs.

config.AddCommand("watch");

And for WatchCommandOptions. Following options expected to be enabled by default (that similar to dotnet watch command).

Currently it inherited BuildCommandOptions though Personally I though dotnet watch command should have different set of command line parameters. (e.g. --no-watch)

@yufeih I'm also want to add feature that relating dotnet watch command. Is it able to create dedicated feature branch?

rmannibucau commented 1 week ago

Yes addcommand is needed but was awaiting to ensure solution was ok.

About build command i guess it would be neat to extract settings in a shared base setting class to ensure it is consistent accross command and not duplicated, wdyt?

edit: created a shared build command options class to host what is shared, moved watch/serve/openbrowser to command specific option classes and set watch/serve to true by default on watch command (not openbrowser since this one is more bothering than helping if you restart the command from time to time). Not sure the intent of --no-watch for a watch command so didn't tackled it - assuming it is exclusions, I think we would need another pass after this PR can be merged to work on the glob to watch pattern conversion but sounds a lot to do it all at once.