-
Notifications
You must be signed in to change notification settings - Fork 238
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
Live Reloading #9
Conversation
Looks like a nice approach! :) I can fix the routes TODO after merging, so don't worry about that for now. |
- remove temporary routes fix (to be fixed in separate MR) - remove unnecessary sqlite-utils requirement
reload logicI've updated the reload logic so that it attempts a reload every 250ms up to 5 seconds before giving up. Hopefully that covers most use cases 🤞 local static reloadsI tested Here's the setup I used for those tests:
app script
tests & documentationthoughts on what tests and docs should be included? |
You shouldn't need any of that, due to this: https://github.com/AnswerDotAI/fasthtml/blob/main/fasthtml/core.py#L254 Here's an example of use: https://github.com/AnswerDotAI/fasthtml/blob/main/examples/db_app.py#L16
Might be hard to test automatically - but there's not much logic there, so maybe not needed? I think just some basic docs in index.ipynb on how to use it would be sufficient. Sound OK? |
Sounds good. Added a
True. As a sanity check I would like to add the basic test shown below. It doesn't cover everything but might catch some regressions in the future. Test# create a regular app with a "/hi" route
app = FastHTML()
cli = TestClient(app)
rt = app.route
@rt("/hi")
def get(): return 'Hi there'
# re-create the same app using live reloading
lr_app = FastHTMLWithLiveReload()
lr_cli = TestClient(app)
lr_rt = lr_app.route
@lr_rt("/hi")
def get(): return 'Hi there'
# check that live reloading does NOT change the underlying behvaiour
test_eq(cli.get('/hi').text, "Hi there")
test_eq(cli.get('/hi').text, lr_cli.get('/hi').text)
# check that live reloading adds the expected route and header script
hdrs, routes = app.router.hdrs, app.routes
lr_hdrs, lr_routes = lr_app.router.hdrs, lr_app.routes
test_eq(len(lr_hdrs), len(hdrs)+1)
test_eq(lr_hdrs[0], lr_app.LIVE_RELOAD_HEADER)
test_eq(len(lr_routes), len(routes)+1)
test_eq(lr_routes[0], lr_app.LIVE_RELOAD_ROUTE) I didn't include the test in the latest commit because it will fail (due to the routes issue). Let me know if you would prefer to commit the failing test now or include it in the PR for the routes fix. |
lgtm! thanks :D |
@comhar I've fixed the routes and added the test so please take a look and check it looks OK. |
Sweet! I ran the latest commits locally and the live reloading is working as expected 👍 |
This PR includes a basic implementation of live reloading.
How does the implementation work?
A subclass of FastHTML (FastHTMLWithLiveReload) does the following:
/live-reload
LIVE_RELOAD_SCRIPT
to each webpageHere are some items to resolve: