fix: keep reloading after a cancelled "Leave site?" dialog

.changeset/fix-beforeunload-cancel-blocks-reload.md

@@ -0,0 +1,5 @@
+---
+"webpack-dev-server": patch
+---
+
+Keep hot and live reload working after a cancelled "Leave site?" dialog.

client-src/index.js

@@ -276,8 +276,41 @@ const logEnabledFeatures = (features) => {
 
 logEnabledFeatures(enabledFeatures);
 
+// `beforeunload` only means the page *may* be leaving: any listener can cancel
+// it, and a cancelled unload fires no event of its own to say so. `pagehide`
+// means it really is going, but only arrives once the next document loads, so
+// nothing tells the two apart at a fixed moment — hence a grace period rather
+// than suppressing until `pagehide`. Too short and a slow navigation can still
+// be interrupted by a reload (#544); too long and a cancelled dialog leaves
+// updates silently dropped (#5571).
+const UNLOAD_GRACE_PERIOD = 2000;
+
+/** @type {ReturnType<typeof setTimeout> | undefined} */
+let unloadGraceTimer;
+
 self.addEventListener("beforeunload", () => {
   status.isUnloading = true;
+
+  clearTimeout(unloadGraceTimer);
+
+  unloadGraceTimer = setTimeout(() => {
+    status.isUnloading = false;
+  }, UNLOAD_GRACE_PERIOD);
+});
+
+// The page really is going now, so stop reloading it for good.
+self.addEventListener("pagehide", () => {
+  clearTimeout(unloadGraceTimer);
+
+  status.isUnloading = true;
+});
+
+// Restored from the back/forward cache: the same script keeps running, so a
+// flag left set by the navigation away would block updates from here on.
+self.addEventListener("pageshow", () => {
+  clearTimeout(unloadGraceTimer);
+
+  status.isUnloading = false;
 });
 
 const overlay =

test/client/index.test.js

@@ -320,4 +320,79 @@ describe("index", () => {
     t.assert.snapshot(log.log.info.mock.calls[1][0]);
     t.assert.snapshot(sendMessage.mock.calls[0][0]);
   });
+
+  describe("unloading", () => {
+    // Longer than the client's own grace period, so a suppression that should
+    // have lapsed has had every chance to.
+    const AFTER_GRACE_PERIOD = 2500;
+
+    const sleep = (ms) =>
+      new Promise((resolve) => {
+        setTimeout(resolve, ms);
+      });
+
+    /**
+     * Drives a live reload to the point where only `isUnloading` can stop it.
+     * @returns {Promise<boolean>} whether the page was reloaded
+     */
+    async function pageReloads() {
+      self.location.reload.mockReset();
+
+      onSocketMessage.liveReload();
+      onSocketMessage.hash(`hash-${Math.random()}`);
+      onSocketMessage.ok();
+
+      // The live reload path polls for a usable window on an interval rather
+      // than reloading straight away, so a reload that is coming needs a few
+      // turns, and one that is suppressed never arrives at all.
+      for (let i = 0; i < 20; i++) {
+        if (self.location.reload.mock.calls.length > 0) return true;
+
+        await sleep(5);
+      }
+
+      return false;
+    }
+
+    it("should reload with no unload in progress", async () => {
+      expect(await pageReloads()).toBe(true);
+    });
+
+    it("should not reload while the page may be leaving", async () => {
+      self.dispatchEvent(new Event("beforeunload"));
+
+      expect(await pageReloads()).toBe(false);
+    });
+
+    it("should reload again once a cancelled unload has lapsed", async () => {
+      self.dispatchEvent(new Event("beforeunload"));
+
+      expect(await pageReloads()).toBe(false);
+
+      // No `pagehide` follows a cancelled dialog, so this is the page staying.
+      await sleep(AFTER_GRACE_PERIOD);
+
+      expect(await pageReloads()).toBe(true);
+    });
+
+    it("should not reload once the page is really gone", async () => {
+      self.dispatchEvent(new Event("beforeunload"));
+      self.dispatchEvent(new Event("pagehide"));
+
+      await sleep(AFTER_GRACE_PERIOD);
+
+      expect(await pageReloads()).toBe(false);
+    });
+
+    it("should reload after a restore from the back/forward cache", async () => {
+      self.dispatchEvent(new Event("beforeunload"));
+      self.dispatchEvent(new Event("pagehide"));
+
+      expect(await pageReloads()).toBe(false);
+
+      self.dispatchEvent(new Event("pageshow"));
+
+      expect(await pageReloads()).toBe(true);
+    });
+  });
 });

test/e2e/overlay.test.js

@@ -372,16 +372,32 @@ describe("overlay", () => {
         }),
       );
 
+      // Marks this document so the wait below can tell it from the reloaded one.
+      await page.evaluate(() => {
+        globalThis.documentFromBeforeTheFix = true;
+      });
+
       fs.writeFileSync(pathToOverlayFixture, overlayFixtureCode);
 
-      await page.waitForSelector("#webpack-dev-server-client-overlay", {
-        hidden: true,
-      });
+      // This fixture builds, so the client dismisses the overlay when `invalid`
+      // announces the rebuild and then live reloads the page once that build
+      // lands. Reading on the dismiss reads the document from before the fix
+      // and races the reload that follows, which is what destroyed the
+      // execution context mid-evaluate. Wait for the reloaded page instead.
+      await waitForExpect(async () => {
+        const reloaded = await page.evaluate(
+          () => globalThis.documentFromBeforeTheFix === undefined,
+        );
 
-      pageHtml = await page.evaluate(() => document.body.outerHTML);
-      overlayHandle = await page.$("#webpack-dev-server-client-overlay");
+        expect(reloaded).toBe(true);
+
+        overlayHandle = await page.$("#webpack-dev-server-client-overlay");
+
+        expect(overlayHandle).toBeNull();
+
+        pageHtml = await page.evaluate(() => document.body.outerHTML);
+      }, 60000);
 
-      expect(overlayHandle).toBeNull();
       t.assert.snapshot(
         await format(pageHtml, {
           parser: "html",
@@ -442,18 +458,29 @@ describe("overlay", () => {
         }),
       );
 
+      const firstErrorOverlayHtml = overlayHtml;
+
       fs.writeFileSync(pathToOverlayFixture, "`;a");
 
-      await page.waitForSelector("#webpack-dev-server-client-overlay", {
-        hidden: true,
-      });
-      await page.waitForSelector("#webpack-dev-server-client-overlay");
+      // The `invalid` message announcing the rebuild dismisses the overlay and
+      // the errors it produces show it again, so the hidden state in between
+      // lasts only as long as that build and is not reliably observable —
+      // waiting for it is a race this test loses as a two minute timeout. What
+      // is under test is the second, different error reaching the overlay.
+      await waitForExpect(async () => {
+        overlayHandle = await page.$("#webpack-dev-server-client-overlay");
 
-      overlayHandle = await page.$("#webpack-dev-server-client-overlay");
-      pageHtml = await page.evaluate(() => document.body.outerHTML);
+        expect(overlayHandle).not.toBeNull();
+
+        overlayFrame = await overlayHandle.contentFrame();
+        overlayHtml = await overlayFrame.evaluate(
+          () => document.body.outerHTML,
+        );
+
+        expect(overlayHtml).not.toBe(firstErrorOverlayHtml);
+      }, 60000);
 
-      overlayFrame = await overlayHandle.contentFrame();
-      overlayHtml = await overlayFrame.evaluate(() => document.body.outerHTML);
+      pageHtml = await page.evaluate(() => document.body.outerHTML);
 
       t.assert.snapshot(
         await format(pageHtml, {
@@ -466,16 +493,32 @@ describe("overlay", () => {
         }),
       );
 
+      // Marks this document so the wait below can tell it from the reloaded one.
+      await page.evaluate(() => {
+        globalThis.documentFromBeforeTheFix = true;
+      });
+
       fs.writeFileSync(pathToOverlayFixture, overlayFixtureCode);
 
-      await page.waitForSelector("#webpack-dev-server-client-overlay", {
-        hidden: true,
-      });
+      // This fixture builds, so the client dismisses the overlay when `invalid`
+      // announces the rebuild and then live reloads the page once that build
+      // lands. Reading on the dismiss reads the document from before the fix
+      // and races the reload that follows, which is what destroyed the
+      // execution context mid-evaluate. Wait for the reloaded page instead.
+      await waitForExpect(async () => {
+        const reloaded = await page.evaluate(
+          () => globalThis.documentFromBeforeTheFix === undefined,
+        );
 
-      pageHtml = await page.evaluate(() => document.body.outerHTML);
-      overlayHandle = await page.$("#webpack-dev-server-client-overlay");
+        expect(reloaded).toBe(true);
+
+        overlayHandle = await page.$("#webpack-dev-server-client-overlay");
+
+        expect(overlayHandle).toBeNull();
+
+        pageHtml = await page.evaluate(() => document.body.outerHTML);
+      }, 60000);
 
-      expect(overlayHandle).toBeNull();
       t.assert.snapshot(
         await format(pageHtml, {
           parser: "html",