getzola / zola

A fast static site generator in a single binary with everything built-in. https://www.getzola.org
https://www.getzola.org
MIT License
13.53k stars 945 forks source link

Update notify dependency #2077

Open Keats opened 1 year ago

Keats commented 1 year ago

v5 has been released and it involves some changes.

The main one is that with the debounced version you do not get the type of event which is probably fine since we don't really care about that.

We do want to only react to a single file in the debounce time though: we should get all the changes to print them but then only do the minimum required. Eg if 3 Sass files have been changed, we should print those 3 files that have changed but only recompile Sass once.

If a Sass file and a Content file have changed, we can skip the Sass compilation since it will be done when rebuilding the content.

Kidsan commented 1 year ago

I would like to have a go at this but I'm not sure what approach to take for collecting the received messages. My understanding is that the watcher will send a message for each individual file changed. Should we log the changed files and cache these received messages, and then evaluate that cache separately or is there an easier approach that I am not seeing

Keats commented 1 year ago

Yeah I'm not entirely sure to be honest. Maybe we can write our own debouncer that handles that rather than use the notify mini debouncer?

0xpr03 commented 1 year ago

If you mean "report only once per interval": debouncer-mini does that by default (otherwise it's a bug). There is also debouncer-full if you need more features.

Keats commented 1 year ago

Can we get a list of all the debounced events in the interval? We want to do some additional filtering after, eg if 10 different templates files have been modified, we want to reload the templates only once.

0xpr03 commented 1 year ago

It should report all timed-out events in one interval. So that sounds to me like what you want. But that also means, if you have a timeout of 2s and events come in at 0,1,2,3s; you end up with events at delivered at 2s(event at 0s), 4s(1,2) and after 6s(3). Hope that helps.

Not sure if some "cooldown" window could be added / helps more.

iamorphen commented 4 months ago

I played with this a bit, and the implementation gets messy if we try and be smart about handling a batch of events minimally. We're lucky if the batch of events requires a full site rebuild, because then we can let the rebuild cover all other events. If we have multiple events and none require a site rebuild, we have to add logic to make sure that the union of all changes required is applied correctly, and its this logic that gets messy due to a variety of checks and sensitive ordering (ex: check for full rebuild cases, then partial update cases, etc.).

I have a local implementation based on notify-debouncer-full. I have not read the code of notify-debouncer-mini, but if it is sensitive to the same events that -full is, including opening and closing files, we probably don't want -mini because we can't get at the event type and therefore filter those events out.

I played with behavior on master by using this toy script and a personal project:

echo " " >> config.toml
echo " " >> config.toml
echo " " >> content/blog/2024-05-14.md
echo " " >> content/blog/2024-05-14.md
touch templates/foo.html templates/bar.html
touch sass/foo.scss
touch sass/bar.scss
touch sass/baz.scss

Master shows (truncated in places):

Change detected @ 2024-05-17 20:52:35
-> Config changed. The browser needs to be refreshed to make the changes visible.

Change detected @ 2024-05-17 20:52:36
-> Content changed /foo/site/content/blog/2024-05-14.md

Change detected @ 2024-05-17 20:52:36
-> Template changed /foo/site/templates/foo.html

Change detected @ 2024-05-17 20:52:36
-> Template changed /foo/site/templates/bar.html

Change detected @ 2024-05-17 20:52:36
-> Sass file changed /foo/site/sass/foo.scss

Change detected @ 2024-05-17 20:52:36
-> Sass file changed /foo/site/sass/bar.scss

Change detected @ 2024-05-17 20:52:36
-> Sass file changed /foo/site/sass/baz.scss

In my local usage of notify-debouncer-full, the same script causes the same 7 events to be raised, but they are joined in a single batch.

Are you okay with me just iterating through the events in the batch and applying updates in series for now? That way we can reuse a lot of the existing handling logic in serve, and the diff becomes a bit less messy. I acknowledge that will blast a user with site reloads if they modify a bunch of files in the 1-second debounce window for some reason, but that also appears to be the current behavior on master.

Keats commented 4 months ago

Can we be a bit smart about the changes? Eg only emit one change for any templates and CSS since we do the same thing for each. For all the other changes it's fine to run them in series

iamorphen commented 4 months ago

There will be more logic and special-casing, but in general, definitely doable. When I next have time I'll see how clean I can make it.