Prevent infinite looping in target callbacks (#459)

Prior to this change, adding or removing target elements could result in
an infinite loop of callbacks and mutations.

This commit introduces `ElementObserver.pause()` (and transitively
introduces `AttributeObserver.pause()` and `TokenListObserver.pause()`)
to provide callers with the ability to pause and resume mutation
observation.

When firing `[name]TargetConnected` and `[name]TargetDisconnected`
callbacks, the behind-the-scenes observers are paused.

docs/reference/targets.md

@@ -114,6 +114,11 @@ export default class extends Controller {
 }
 ```
 
+**Note** During the execution of `[name]TargetConnected` and
+`[name]TargetDisconnected` callbacks, the `MutationObserver` instances behind
+the scenes are paused. This means that if a callback add or removes a target
+with a matching name, the corresponding callback _will not_ be invoked again.
+
 ## Naming Conventions
 
 Always use camelCase to specify target names, since they map directly to properties on your controller.

src/core/target_observer.ts

@@ -51,14 +51,14 @@ export class TargetObserver implements TokenListObserverDelegate {
   connectTarget(element: Element, name: string) {
     if (!this.targetsByName.has(name, element)) {
       this.targetsByName.add(name, element)
-      this.delegate.targetConnected(element, name)
+      this.tokenListObserver?.pause(() => this.delegate.targetConnected(element, name))
     }
   }
 
   disconnectTarget(element: Element, name: string) {
     if (this.targetsByName.has(name, element)) {
       this.targetsByName.delete(name, element)
-      this.delegate.targetDisconnected(element, name)
+      this.tokenListObserver?.pause(() => this.delegate.targetDisconnected(element, name))
     }
   }
 

src/mutation-observers/attribute_observer.ts

@@ -31,6 +31,10 @@ export class AttributeObserver implements ElementObserverDelegate {
     this.elementObserver.start()
   }
 
+  pause(callback: () => void) {
+    this.elementObserver.pause(callback)
+  }
+
   stop() {
     this.elementObserver.stop()
   }

src/mutation-observers/element_observer.ts

@@ -14,6 +14,7 @@ export class ElementObserver {
 
   private elements: Set<Element>
   private mutationObserver: MutationObserver
+  private mutationObserverInit = { attributes: true, childList: true, subtree: true }
 
   constructor(element: Element, delegate: ElementObserverDelegate) {
     this.element = element
@@ -27,11 +28,25 @@ export class ElementObserver {
   start() {
     if (!this.started) {
       this.started = true
-      this.mutationObserver.observe(this.element, { attributes: true, childList: true, subtree: true })
+      this.mutationObserver.observe(this.element, this.mutationObserverInit)
       this.refresh()
     }
   }
 
+  pause(callback: () => void) {
+    if (this.started) {
+      this.mutationObserver.disconnect()
+      this.started = false
+    }
+
+    callback()
+
+    if (!this.started) {
+      this.mutationObserver.observe(this.element, this.mutationObserverInit)
+      this.started = true
+    }
+  }
+
   stop() {
     if (this.started) {
       this.mutationObserver.takeRecords()

src/mutation-observers/token_list_observer.ts

@@ -32,6 +32,10 @@ export class TokenListObserver implements AttributeObserverDelegate {
     this.attributeObserver.start()
   }
 
+  pause(callback: () => void) {
+    this.attributeObserver.pause(callback)
+  }
+
   stop() {
     this.attributeObserver.stop()
   }

src/tests/controllers/target_controller.ts

@@ -10,8 +10,8 @@ class BaseTargetController extends Controller {
 
 export class TargetController extends BaseTargetController {
   static classes = [ "connected", "disconnected" ]
-  static targets = [ "beta", "input" ]
-  static values = { inputTargetConnectedCallCount: Number, inputTargetDisconnectedCallCount: Number }
+  static targets = [ "beta", "input", "recursive" ]
+  static values = { inputTargetConnectedCallCount: Number, inputTargetDisconnectedCallCount: Number, recursiveTargetConnectedCallCount: Number, recursiveTargetDisconnectedCallCount: Number }
 
   betaTarget!: Element | null
   betaTargets!: Element[]
@@ -28,6 +28,8 @@ export class TargetController extends BaseTargetController {
 
   inputTargetConnectedCallCountValue = 0
   inputTargetDisconnectedCallCountValue = 0
+  recursiveTargetConnectedCallCountValue = 0
+  recursiveTargetDisconnectedCallCountValue = 0
 
   inputTargetConnected(element: Element) {
     if (this.hasConnectedClass) element.classList.add(this.connectedClass)
@@ -38,4 +40,15 @@ export class TargetController extends BaseTargetController {
     if (this.hasDisconnectedClass) element.classList.add(this.disconnectedClass)
     this.inputTargetDisconnectedCallCountValue++
   }
+
+  recursiveTargetConnected(element: Element) {
+    element.remove()
+
+    this.recursiveTargetConnectedCallCountValue++
+    this.element.append(element)
+  }
+
+  recursiveTargetDisconnected(element: Element) {
+    this.recursiveTargetDisconnectedCallCountValue++
+  }
 }

src/tests/modules/core/target_tests.ts

@@ -164,4 +164,15 @@ export default class TargetTests extends ControllerTestCase(TargetController) {
     this.assert.ok(element.classList.contains("disconnected"), `expected "${element.className}" to contain "disconnected"`)
     this.assert.ok(element.isConnected, "element is still present in document")
   }
+
+  async "test [target]Connected() and [target]Disconnected() do not loop infinitely"() {
+    this.controller.element.insertAdjacentHTML("beforeend", `
+      <div data-${this.identifier}-target="recursive" id="recursive2"></div>
+    `)
+    await this.nextFrame
+
+    this.assert.ok(!!this.fixtureElement.querySelector("#recursive2"))
+    this.assert.equal(this.controller.recursiveTargetConnectedCallCountValue, 1)
+    this.assert.equal(this.controller.recursiveTargetDisconnectedCallCountValue, 0)
+  }
 }