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 a Dream.livereload middleware #279

Merged
merged 3 commits into from
Jun 1, 2023
Merged

Conversation

tmattio
Copy link
Contributor

@tmattio tmattio commented May 13, 2023

The code is adapted from dream-livereload, itself adapted from the w-live-reload example.
We update the w-live-reload to use the newly introduced middleware.

@aantron
Copy link
Owner

aantron commented May 15, 2023

Thanks!

Note: suggested here, just so the issue has a backlink.

@aantron
Copy link
Owner

aantron commented May 15, 2023

@tmattio, I think we should remove the unit argument. The unit argument makes Dream.livereload () compose better with Dream.no_middleware inside an if-expression. But we already have middlewares that may be used in development but that don't have the unit argument and therefore don't compose well inside if, for example, Dream.memory_sessions:

val memory_sessions : ?lifetime:float -> middleware

Also, this approach is not future-proof, because any middleware could gain an optional argument in the future, and we won't be able to add unit arguments to them without breaking users. This does mean that adding any optionals to any middlewares will break them if they are used inside if-expressions, but that's just a general flaw of optional arguments in OCaml.

We will just add an Dream.if_ combinator later if needed. For example:

let if_ : bool -> then_:Dream.middleware -> Dream.middleware =
    fun condition ~then_ ->
  if condition then
    then_ 
  else
    Dream.no_middleware

It's also possible for the user to work around this using a type constraint, Dream.livereload : middleware, although, of course, this is awkward.

Separately, are the ?script and ?path options really needed? We can probably get by for now without them. I'd rather add them later on request, than support them when there is no evidence of them being used (I mean, I don't have any -- you may well have!).

src/server/livereload.ml Outdated Show resolved Hide resolved
src/server/livereload.ml Outdated Show resolved Hide resolved
src/server/dune Show resolved Hide resolved
example/w-live-reload/README.md Show resolved Hide resolved
src/server/livereload.ml Outdated Show resolved Hide resolved
src/server/livereload.ml Outdated Show resolved Hide resolved
@aantron
Copy link
Owner

aantron commented May 15, 2023

@tmattio, Thanks for the PR! We left you a bunch of comments, but in case you are pressed for time, etc., I'm ready to take over the PR, push in a bunch of small changes and squash it down, crediting you :)

@tmattio
Copy link
Contributor Author

tmattio commented May 17, 2023

Thanks for the review @aantron (et al. 😉)!

I'm happy to continue with the PR and address your comments but probably won't have time before next week - if you have time before that for sure feel free to take over, otherwise, I'll push patches as soon as possible

@aantron
Copy link
Owner

aantron commented Jun 1, 2023

@tmattio, I'm taking over this PR to merge it in!

tmattio and others added 3 commits June 1, 2023 16:23
The code is adapted from dream-livereload, itself adapted from the w-live-reload example.
We update the w-live-reload to use the newly introduced middleware.

Apply nits
@aantron aantron merged commit 10ae36f into aantron:master Jun 1, 2023
@aantron
Copy link
Owner

aantron commented Jun 1, 2023

Thank you!

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.

3 participants