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

fix: stale live reload due to dropped watch events #649

Merged

Conversation

marvinhagemeister
Copy link
Contributor

@marvinhagemeister marvinhagemeister commented Aug 13, 2024

Description

While working on docs.deno.com we noticed that the page shown in the browser would often go out of sync with the actual content on disk. Debugging this further lead two a discovery of two separate bugs in the watcher + live reloading code.

When you edit a file and hit save while a previous change is still being processed it would be dropped here:

lume/core/watcher.ts

Lines 87 to 89 in abefe9f

if (!changes.size || runningCallback) {
return;
}

This lead to generated files lagging behind one or potentially more changes depending on how many watch events were dropped. I've resolved this by adding a queue of events that is processed in order. To avoid processing the same file more often than necessary there is a small optimisation that merges queued events if they are the same.

But with this fix in place the browser was still showing a previous version of the page. Turns out that we frequently run into the situation where a watch event is received, whilst the browser is disconnected and is in the process of being reloaded due to an earlier change. These events were dropped here:

watcher.addEventListener("change", (event) => {
if (!sockets.size) {
return;
}

To resolve this, I've added a simple "revision" tracking mechanism. When a new connection is opened the server immediately sends the current revision to the browser so that it can check if it has become stale and potentially reload the page. The initial revision is included in the HTML and passed to the liveReload() function as an argument.

Before

Notice how even refreshing the browser shows the stale version.

lume-watch-before.mp4

After

Notice how the changes are all sent to the browser and it has the correct state even if you reload it. It seems like during processing the lume server is blocked from responding to the browser, otherwise the intermediate states would also be shown in it.

lume-watch-after.mp4

Related Issues

Check List

  • Have you read the
    CODE OF CONDUCT
  • Have you read the document
    CONTRIBUTING
    • One pull request per feature. If you want to do more than one thing,
      send multiple pull request.
    • Write tests.
    • Run deno fmt to fix the code format before commit.
    • Document any change in the CHANGELOG.md.

let changes: Set<string> | undefined;
while ((changes = changeQueue.pop()) !== undefined) {
try {
const result = await this.dispatchEvent({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of dispatching an event per change in the queue, why don't merge all changes and dispatch the event only once? I mean:

let changes: Set<string> | undefined;
let mergedChanges = new Set<string>();

while ((changes = changeQueue.pop()) !== undefined) {
  mergedChanges = mergedChanges.union(changes);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it didn't even occur to me that we could batch all future changes into the same one. This is a great idea!

@oscarotero
Copy link
Member

Hey @marvinhagemeister
The changes look great, thanks so much!
I just let a comment to optimize the number of times the event "change" is invoked. Let me know if it makes sense.

@oscarotero oscarotero merged commit c6eb770 into lumeland:main Aug 14, 2024
2 of 3 checks passed
@oscarotero
Copy link
Member

thank you for this great improvement!

@marvinhagemeister marvinhagemeister deleted the fix-watcher-dropping-events branch August 14, 2024 09:49
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