Recommend watchexec for live_reload example

[thangngoc89]

I have been trying several solution over the past few days and so far, watchexec (https://github.com/watchexec/watchexec) is the most reliable one and it’s fast.

fswatch + custom script in the current example keep failing to kill the process, I have to kill it manually several times

Example:

watchexec -e re,ml,rei,mli -r -- esy dune exec ./main.exe

[tcoopman]

I haven't looked at this, but shouldn't we be able to use dune --watch for this?

[thangngoc89]

@tcoopman I think the reason have been explained in live-reload example. dune —watch doesn’t kill the server process

[tcoopman]

ok, just found the issue on dune: ocaml/dune#2934

[aantron]

Also cc @tmattio, in case watchexec is good for the example script in dream-liverelaod.

[aantron]

From #139 (comment), a command line for killing any listening Dream processes (seemingly using port 5000) and restarting:

lsof -i tcp:5000 | grep LISTEN | awk '{print $2}' | xargs kill -9 && dune exec ./src/main.exe

[cemerick]

It seems that the dune limitation is confined to dune exec. I've had good results by setting up a "test" executable and alias (my dream app needs a number of small tweaks for a dev/test context anyway):

(executable (name runapp)
  (libraries supervisor)
  (modules Runapp)
  (flags (:standard -g))
  (preprocess (pps ppx_deriving.std)))

(rule
  (alias runapp)
  (deps runapp.exe)
  (action (run ./runapp.exe)))

and then I run it with dune build @runapp -w --force --no-buffer. No need for an additional filesystem watcher, explicit process reaping, etc.

[aantron]

Does this automatically kill a running runapp.exe whenever you make changes to the code?

[cemerick]

So far in my experience, yes. I've not used fswatch at all.

[dangdennis]

Can close because dune exec -w is now available

[aantron]

Thanks! I just tried this out and it seems to kill the running server now -- very nice! We probably need to update the watch example.

[tmattio]

In case it's useful, I created a demo of a live reload workflow using Dune's watch mode for executables at https://github.com/tmattio/dune-watchmode-livereload-demo.

[aantron]

Very nice! With Dune's server-killing watch support and the upcoming Dune 3.8.0 fix that will print a newline before build output (and thus avoid making the first line of the log very ugly, that first line typically printing localhost:8080 for the developer to click on), we should probably merge live reload into "core" Dream so that it all works out of the box for a very nice experience. @tmattio, what do you think?

[aantron]

a newline before build output

That is, a newline after build output and before running the executable being built.

[tmattio]

Sure, I'm happy to open a PR for that 🙂

[aantron]

I took a look at the dream-livereload repo again, and with all these improvements, we can get rid of the shell script completely. We can also combine the route and middleware into just a middleware that both injects the script, and intercepts requests by matching on the target (as long as we document that). That would make usage very easy, and maintain the "one feature/aspect is activated in one place" goal that Dream is aiming for. It would introduce Markup.ml and Lambda Soup dependency to Dream, but both of those are "pure" and not more heavy than, say, the base Caqti dependency that we already have (which is also "pure," the other Caqti plugins introduce system dependencies on database libs).

[aantron]

The middleware can probably have an optional ?route argument so that the user can move it to wherever they need it in their app, in case of conflict. Actually, I would not add that, but just keep that idea in reserve in case it becomes needed. And an ?enabled argument would allow disabling the middleware outside of production without some kind of kludge rebuilding the middleware stack depending on whether this is developer mode or production.

[aantron]

The only wrinkle I see is that the middleware probably needs to respect the site prefix, but I would merge a PR without that. That part of the router probably needs to be factored out at this point, then, so that middlewares like this can reuse that code.

[aantron]

Actually, scratch ?enabled in favor of Dream.no_middleware. I think the Testing section is the best place for this val to go, and I will then link to it in the README because this is probably one of the most important development middlewares. Maybe we will rename Testing to Development later.

[tmattio]

All good points, thanks! I'll keep them in mind before making the PR.

Re. the ?enabled argument, I'm thinking that without it, user code will end up looking like:

if development then
  Dream.run
  ...
  list_of_middlewares
  ...
else
  Dream.run
  ...
  list_of_middlewares
  ...

Or having if-else statements with Dream.no_middlware, but that's also quite sparse.

Passing enabled:is_development in select middleware seems more ergonomic and concise. What do you think?

[aantron]

I do agree with that, but I wanted to stick with Dream.no_middleware for now. It is definitely uglier, but it is more composable from the Dream developers' point of view, as we don't have to go through middlewares and consider potentially adding ?enabled consistently to each. So I'd rather also keep that in reserve until it becomes really clear that we need it.

There is one ergonomics counterargument to ?enabled on all the middlewares, which is that if I have a whole stack of development middlewares, I'd rather actually wrap if development then Dream.pipeline [...] else Dream.no_middleware rather than do

@@ middleware_1 ?enabled:is_development
@@ middleware_2 ?enabled:is_development
@@ middleware_3 ?enabled:is_development

though I'm not sure yet :) But it does seem more maintainable even for me as the user to have the condition in one place, and the slightly awkward but still composable approach allows that. Although that's also possible if we adopt both approaches.

In summary, it seems to me like it's not clear that ?enabled is worth it, yet, though it might be.