From 9944490a3c8aec0c5060401125cc8932e93a32df Mon Sep 17 00:00:00 2001
From: Jorge Manrubia <jorge@hey.com>
Date: Mon, 30 Oct 2023 14:29:23 +0100
Subject: [PATCH] Don't reload stimulus controllers after morphing

There was a flaw in the implementation: we wanted to reload the stimulus controllers
when their element was effectively morphed because some attribute had changed. Our
implementation was essentially reloading all the stimulus controllers instead.

But, even if we implemented our original idea, we have changed our mind about it being
the right call. The heuristic of "reload controllers when some attribute changed" came
from some tests with legacy controllers that used dom attributes to track certain
conditions. That doesn't seem like enough justification for the original idea.

In general, you don't want to reload controllers unless their elements get disconnected
or connected as part of the morphing operation. If it's important for a given controller
to track changes to the dom, than it should do that (e.g: listening to connection of targets or
outlets, or just with the mutation observer API), but we can't determine that from the outside.

If we introduce some API here, it will be the opposite: an API to force a "reconnect" during
morphing, but we need to see a real justification in practice.
---
 src/core/drive/morph_renderer.js           | 14 ++------------
 src/tests/functional/page_refresh_tests.js | 18 ------------------
 2 files changed, 2 insertions(+), 30 deletions(-)

diff --git a/src/core/drive/morph_renderer.js b/src/core/drive/morph_renderer.js
index 6537b8d2f..7ce1d2ed6 100644
--- a/src/core/drive/morph_renderer.js
+++ b/src/core/drive/morph_renderer.js
@@ -1,5 +1,5 @@
 import Idiomorph from "idiomorph"
-import { dispatch, nextAnimationFrame } from "../../util"
+import { dispatch } from "../../util"
 import { Renderer } from "../renderer"
 
 export class MorphRenderer extends Renderer {
@@ -33,8 +33,7 @@ export class MorphRenderer extends Renderer {
       callbacks: {
         beforeNodeAdded: this.#shouldAddElement,
         beforeNodeMorphed: this.#shouldMorphElement,
-        beforeNodeRemoved: this.#shouldRemoveElement,
-        afterNodeMorphed: this.#reloadStimulusControllers
+        beforeNodeRemoved: this.#shouldRemoveElement
       }
     })
   }
@@ -78,15 +77,6 @@ export class MorphRenderer extends Renderer {
     this.#morphElements(currentElement, newElement.children, "innerHTML")
   }
 
-  #reloadStimulusControllers = async (node) => {
-    if (node instanceof HTMLElement && node.hasAttribute("data-controller")) {
-      const originalAttribute = node.getAttribute("data-controller")
-      node.removeAttribute("data-controller")
-      await nextAnimationFrame()
-      node.setAttribute("data-controller", originalAttribute)
-    }
-  }
-
   #isRemoteFrame(element) {
     return element.nodeName.toLowerCase() === "turbo-frame" && element.src
   }
diff --git a/src/tests/functional/page_refresh_tests.js b/src/tests/functional/page_refresh_tests.js
index 4d657db7f..272a34c09 100644
--- a/src/tests/functional/page_refresh_tests.js
+++ b/src/tests/functional/page_refresh_tests.js
@@ -1,6 +1,5 @@
 import { test, expect } from "@playwright/test"
 import {
-  nextAttributeMutationNamed,
   nextBeat,
   nextEventNamed,
   nextEventOnTarget,
@@ -127,23 +126,6 @@ test("it preserves data-turbo-permanent elements that don't match when their ids
   await expect(page.locator("#preserve-me")).toHaveText("Preserve me, I have a family!")
 })
 
-test("it reloads data-controller attributes after a morph", async ({ page }) => {
-  await page.goto("/src/tests/fixtures/page_refresh.html")
-
-  await page.click("#form-submit")
-  await nextEventNamed(page, "turbo:render", { renderMethod: "morph" })
-
-  expect(
-    await nextAttributeMutationNamed(page, "stimulus-controller", "data-controller")
-  ).toEqual(null)
-
-  await nextBeat()
-
-  expect(
-    await nextAttributeMutationNamed(page, "stimulus-controller", "data-controller")
-  ).toEqual("test")
-})
-
 async function assertPageScroll(page, top, left) {
   const [scrollTop, scrollLeft] = await page.evaluate(() => {
     return [