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

[Pitch] Manual reload #42

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pcantrell
Copy link
Contributor

@pcantrell pcantrell commented Jan 13, 2025

Detailed description

This is a possible feature. I’m opening the PR to see (1) whether the general idea would be acceptable for the project, and (2) if so, what adjustments this specific design needs.

This PR contains a working implementation, but no docs or tests yet (pending feedback).

The problem

I’m using adsf for a custom static site generator, which does not have an incremental rebuild: it always regenerates all the output files when any file changes. That mixes poorly with adsf-live, which watches and reports every individual file change, causing several problems:

  • Using Listen with many files at once has performance costs.

  • Sending the paths for every output file in the whole site on every change has performance costs.

    In my project, this plus the previous item make rebuilding on change slower by a factor of 2-3x than a clean build (before the watcher has started). Furthermore, it’s unclear why, but this performance penalty appears to increase with multiple rebuilds. Reporting individual file changes is supposed to improve performance, but for me has the opposite effect.

  • Change notifications to the client start before the output directory is fully written, frequently causing multiple client refreshes for a single rebuild.

  • Files that are written incrementally sometimes get refreshed by the browser before fully written, resulting in (for example) pages loaded with only half the stylesheet, forcing a manual refresh (and thus defeating the whole purpose of adsf-live).

These problems all boil down to three causes:

  • Watching so many files is a problem.
  • Sending paths of so many individual files to the browser is a problem.
  • Triggering refresh on “individual file changed!” instead of “build complete!” is a problem.

The proposal

This PR implements three related features:

  1. The Adsf server now supports a “manual” live reload mode, enabled like this:
server = Adsf::Server.new(live: :manual, root: )

In manual mode, adsf does not watch for file changes.

  1. Adsf::Server now has a live_reload method, which optionally accepts an array of paths, but by default signals that all files need to be reloaded:
server.live_reload  # reload everything
server.live_reload(["/some/changed/file", "/other/changed/file"])
  1. In the socket server protocol, passing null for path now signals that the browser should always reload, regardless of the currently displayed file.

(This would be a Ruby-API-only feature; I don’t think it’s urgent for the CLI. The CLI could support it by triggering reload when input arrives on stdin, or by accepting a UNIX pipe argument, or by watching one specific reload file or something…but that feels like a separate feature request that needs a use case first.)

What do you think? Is this a feature idea you’d accept? If so, beyond docs and tests (obviously), what PR changes would you like to see? Would you prefer a different design? (I’m particularly uncertain about the API shape for (1) and the change to the protocol in (3).)

To do

  • Tests
  • Documentation
  • Feature flags

Related issues

None found.

@denisdefreyne
Copy link
Owner

I like this idea! I need a bit more time to look at it properly.

I can see an advantage for Nanoc as well: since it already knows which paths are being updated, it would likely be able to more accurately reload the browser as well.

@pcantrell
Copy link
Contributor Author

Mull it over, and I’m happy to make adjustments once you’ve formulated your thoughts. I’m happily using my fork in the meantime.

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.

2 participants