luckyframework / lucky

A full-featured Crystal web framework that catches bugs for you, runs incredibly fast, and helps you write code that lasts.
https://luckyframework.org
MIT License
2.57k stars 156 forks source link

Live-Reload for lucky watch #1693

Closed mdwagner closed 2 years ago

mdwagner commented 2 years ago

Purpose

https://github.com/luckyframework/lucky/issues/1692

Description

Changes lucky watch to reload the browser with either Browsersync, WebSockets, or SSE. Adds reload_port option to watch.yml (defaults to 3001).

Example:

$ lucky watch # should work as before
$ lucky watch --reload-browser # defaults to WebSockets for watcher
$ lucky watch -r -w sse  # shorthands, using ServerSentEvents for watcher
$ lucky watch -r --watcher=browsersync # using Browsersync for watcher

Checklist

mdwagner commented 2 years ago

I started playing with the idea of also adding SSE besides WebSockets. Here's my example at https://github.com/mdwagner/sse-cr-example

mdwagner commented 2 years ago

So, I decided to make the SSE version as well. Now the user has 3 options for reloading: WebSockets (new default), Browsersync (for existing projects), and ServerSentEvents (SSE). Now I just need to try each of them out for validation...

UPDATE: After getting my local env setup, I was able to refine this and got all 3 working.

mdwagner commented 2 years ago

Yeah, it should work for API only apps, although I just realized it's not watching for js/css files, so I could add that in (only needed for ws or sse watchers).

live_reload_connect_tag is only for ws or sse watchers, because browsersync doesn't use it. I agree, it should be added to the lucky_cli skeleton init. We should also update the Procfile.dev to use lucky watch -r default, because right now it will fail because we don't pass it a watcher type.

The only other thing I'm wondering about is whether this lucky watch --reload-browser args API is the right approach... See below

mdwagner commented 2 years ago

@jwoertink You'll need your lucky referencing this forked branch, along with a new lucky.watch binary from this branch.

mdwagner commented 2 years ago

So, I added js/css file patterns (same as in bs-config.js, although this could be improved later in watch.yml, if necessary).

I also decided that lucky watch arguments should make a little more sense than it did previously. Now, lucky watch --reload-browser should continue to work, it just defaults to WebSockets instead of Browsersync. SSE and Browsersync are still there, just behind the --watcher flag (or -w shorthand).

Besides those changes, I think this PR is good to go. But let me know if you think I should move file watcher patterns to watch.yml or not, it could be useful for someone.

mdwagner commented 2 years ago

@jwoertink Thanks! I had fun learning how these things worked!

Yeah, I can open another PR for lucky_cli. One question I have though is whether I should remove Browsersync from the skeleton code? The website documentation could cover when you may want to use Browsersync, so idk if it's necessary to keep it in.

jwoertink commented 2 years ago

Let's leave it in for the next release, and then we can like deprecate it and remove it later.

mdwagner commented 2 years ago

I guess lucky_cli CI will fail until this gets merged, right? Because it doesnt have the new code yet

jwoertink commented 2 years ago

Yeah. LuckyCLI is failing anyway due to some other things, so don't worry about the green there. I'll merge this in now though. Thanks!