From 3128ef1a5861ce4f9bb7c57f3a21bef6a8c0ba7d Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Thu, 7 Oct 2021 03:16:05 -0400 Subject: [PATCH] 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 | 5 +++++ src/core/target_observer.ts | 4 ++-- src/mutation-observers/attribute_observer.ts | 4 ++++ src/mutation-observers/element_observer.ts | 17 ++++++++++++++++- src/mutation-observers/token_list_observer.ts | 4 ++++ src/tests/controllers/target_controller.ts | 17 +++++++++++++++-- src/tests/modules/core/target_tests.ts | 11 +++++++++++ 7 files changed, 57 insertions(+), 5 deletions(-) diff --git a/docs/reference/targets.md b/docs/reference/targets.md index 1ea4d7b2..93151000 100644 --- a/docs/reference/targets.md +++ b/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. diff --git a/src/core/target_observer.ts b/src/core/target_observer.ts index 05230ab9..2f144ef2 100644 --- a/src/core/target_observer.ts +++ b/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)) } } diff --git a/src/mutation-observers/attribute_observer.ts b/src/mutation-observers/attribute_observer.ts index 5e1fd6fa..b0a5181b 100644 --- a/src/mutation-observers/attribute_observer.ts +++ b/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() } diff --git a/src/mutation-observers/element_observer.ts b/src/mutation-observers/element_observer.ts index aa172c97..f07e9450 100644 --- a/src/mutation-observers/element_observer.ts +++ b/src/mutation-observers/element_observer.ts @@ -14,6 +14,7 @@ export class ElementObserver { private elements: Set 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() diff --git a/src/mutation-observers/token_list_observer.ts b/src/mutation-observers/token_list_observer.ts index 88b4a71d..c0db034f 100644 --- a/src/mutation-observers/token_list_observer.ts +++ b/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() } diff --git a/src/tests/controllers/target_controller.ts b/src/tests/controllers/target_controller.ts index ff4835af..8ccadd3d 100644 --- a/src/tests/controllers/target_controller.ts +++ b/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++ + } } diff --git a/src/tests/modules/core/target_tests.ts b/src/tests/modules/core/target_tests.ts index f0f3dc08..40aecec5 100644 --- a/src/tests/modules/core/target_tests.ts +++ b/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", ` +
+ `) + await this.nextFrame + + this.assert.ok(!!this.fixtureElement.querySelector("#recursive2")) + this.assert.equal(this.controller.recursiveTargetConnectedCallCountValue, 1) + this.assert.equal(this.controller.recursiveTargetDisconnectedCallCountValue, 0) + } }