Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Controllers can be added/removed during lifecycle. #4388

Merged
merged 6 commits into from
Nov 13, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/healthy-elephants-itch.md
AndrewJakubowicz marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@lit/reactive-element': patch
---

Controllers can be added/removed during lifecycle (#4266).
7 changes: 7 additions & 0 deletions .changeset/strong-ants-think.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@lit/reactive-element': patch
'lit': patch
'lit-element': patch
---

Controllers can be added/removed during lifecycle (#4266).
augustjk marked this conversation as resolved.
Show resolved Hide resolved
8 changes: 3 additions & 5 deletions packages/reactive-element/src/reactive-element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -993,7 +993,7 @@ export abstract class ReactiveElement
/**
* Set of controllers.
*/
private __controllers?: ReactiveController[];
private __controllers?: Set<ReactiveController>;

constructor() {
super();
Expand Down Expand Up @@ -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
Expand All @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice, looks like Sets are very well behaved when mutated during iteration: https://lit.dev/playground/#gist=f424f3aab96d7c839d43c2c5815c8a10

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
2 changes: 1 addition & 1 deletion scripts/check-size.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = 15423;
const expectedLitCoreSize = 15405;
AndrewJakubowicz marked this conversation as resolved.
Show resolved Hide resolved
const expectedLitHtmlSize = 7250;

const litCoreSrc = fs.readFileSync('packages/lit/lit-core.min.js', 'utf8');
Expand Down