Skip to content

Commit

Permalink
Don't reload stimulus controllers after morphing
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jorgemanrubia committed Oct 30, 2023
1 parent 6777ddf commit 25b9db0
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 30 deletions.
14 changes: 2 additions & 12 deletions src/core/drive/morph_renderer.js
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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
}
})
}
Expand Down Expand Up @@ -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
}
Expand Down
18 changes: 0 additions & 18 deletions src/tests/functional/page_refresh_tests.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { test, expect } from "@playwright/test"
import {
nextAttributeMutationNamed,
nextBeat,
nextEventNamed,
nextEventOnTarget,
Expand Down Expand Up @@ -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 [
Expand Down

0 comments on commit 25b9db0

Please sign in to comment.