[reactive-element] controllers can be added/removed during lifecycle …

…callbacks (#4388)

Fixes #4266. Controllers are maintained via a Set
instead of an Array, allowing them to be iterated
stably while being modified.

Co-authored-by: Augustine Kim <augustinekim@google.com>

.changeset/strong-ants-think.md

@@ -0,0 +1,7 @@
+---
+'@lit/reactive-element': patch
+'lit': patch
+'lit-element': patch
+---
+
+Fixes bug where adding or removing controllers during a reactive controller lifecycle would affect the execution of other controllers (#4266). Controllers can now be added/removed during lifecycle without affecting others.

packages/reactive-element/src/reactive-element.ts

@@ -993,7 +993,7 @@ export abstract class ReactiveElement
   /**
    * Set of controllers.
    */
-  private __controllers?: ReactiveController[];
+  private __controllers?: Set<ReactiveController>;
 
   constructor() {
     super();
@@ -1030,7 +1030,7 @@ export abstract class ReactiveElement
    * @category controllers
    */
   addController(controller: ReactiveController) {
-    (this.__controllers ??= []).push(controller);
+    (this.__controllers ??= new Set()).add(controller);
     // If a controller is added after the element has been connected,
     // call hostConnected. Note, re-using existence of `renderRoot` here
     // (which is set in connectedCallback) to avoid the need to track a
@@ -1045,9 +1045,7 @@ export abstract class ReactiveElement
    * @category controllers
    */
   removeController(controller: ReactiveController) {
-    // Note, if the indexOf is -1, the >>> will flip the sign which makes the
-    // splice do nothing.
-    this.__controllers?.splice(this.__controllers.indexOf(controller) >>> 0, 1);
+    this.__controllers?.delete(controller);
   }
 
   /**

packages/reactive-element/src/test/reactive-element-controllers_test.ts

@@ -201,4 +201,75 @@ suite('Reactive controllers', () => {
     assert.equal(el.controller.connectedCount, 2);
     assert.equal(el.controller.disconnectedCount, 1);
   });
+
+  test('controllers can be removed during lifecycle', async () => {
+    class RemovingController implements ReactiveController {
+      host: ReactiveElement;
+      updatedCount = 0;
+
+      constructor(host: ReactiveElement) {
+        this.host = host;
+        this.host.addController(this);
+      }
+
+      hostUpdated() {
+        this.updatedCount++;
+        this.host.removeController(this);
+      }
+    }
+    const removingController = new RemovingController(el);
+    const controller = new MyController(el);
+    assert.equal(el.controller.updatedCount, 1);
+    assert.equal(removingController.updatedCount, 0);
+    assert.equal(controller.updatedCount, 0);
+    el.requestUpdate();
+    await el.updateComplete;
+    assert.equal(el.controller.updatedCount, 2);
+    assert.equal(removingController.updatedCount, 1);
+    assert.equal(controller.updatedCount, 1);
+    el.requestUpdate();
+    await el.updateComplete;
+    assert.equal(el.controller.updatedCount, 3);
+    assert.equal(removingController.updatedCount, 1);
+    assert.equal(controller.updatedCount, 2);
+  });
+
+  test('controllers can add other controllers during lifecycle', async () => {
+    class AddingController implements ReactiveController {
+      host: ReactiveElement;
+      updateCount = 0;
+
+      controllers?: MyController[];
+
+      constructor(host: ReactiveElement) {
+        this.host = host;
+        this.host.addController(this);
+      }
+
+      hostUpdate() {
+        this.updateCount++;
+        (this.controllers ??= []).push(new MyController(this.host));
+      }
+    }
+    const addingController = new AddingController(el);
+    const controller = new MyController(el);
+    assert.equal(el.controller.updatedCount, 1);
+    assert.equal(addingController.updateCount, 0);
+    assert.equal(controller.updateCount, 0);
+    el.requestUpdate();
+    await el.updateComplete;
+    assert.equal(el.controller.updateCount, 2);
+    assert.equal(addingController.updateCount, 1);
+    assert.equal(addingController.controllers?.length, 1);
+    assert.equal(addingController.controllers?.[0].updateCount, 1);
+    assert.equal(controller.updateCount, 1);
+    el.requestUpdate();
+    await el.updateComplete;
+    assert.equal(el.controller.updateCount, 3);
+    assert.equal(addingController.updateCount, 2);
+    assert.equal(addingController.controllers?.length, 2);
+    assert.equal(addingController.controllers?.[0].updateCount, 2);
+    assert.equal(addingController.controllers?.[1].updateCount, 1);
+    assert.equal(controller.updateCount, 2);
+  });
 });

scripts/check-size.js

@@ -9,7 +9,7 @@ import * as fs from 'fs';
 // it's likely that we'll ask you to investigate ways to reduce the size.
 //
 // In either case, update the size here and push a new commit to your PR.
-const expectedLitCoreSize = 15465;
+const expectedLitCoreSize = 15447;
 const expectedLitHtmlSize = 7250;
 
 const litCoreSrc = fs.readFileSync('packages/lit/lit-core.min.js', 'utf8');