From 344a118985e6473911fe16eee0abc92eaea54ded Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Mon, 20 Jul 2020 09:41:14 +0100 Subject: [PATCH 01/17] test: add bind test that asserts deep children are bound --- test/bind.js | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/bind.js b/test/bind.js index 80590f2a..0a0c9a86 100644 --- a/test/bind.js +++ b/test/bind.js @@ -206,4 +206,24 @@ describe('bind', () => { expect(instance.foo).to.have.been.called.exactly(0) }) }) + + it('re-binds actions deeply in the HTML', async function () { + const instance = document.createElement('bind-test-element') + chai.spy.on(instance, 'foo') + root.appendChild(instance) + listenForBind(root) + instance.innerHTML = ` +
+
+
+
+ ` + // We need to wait for a couple of frames after injecting the HTML into to + // controller so that the actions have been bound to the controller. + await waitForNextAnimationFrame() + await waitForNextAnimationFrame() + instance.querySelector('button').click() + expect(instance.foo).to.have.been.called.exactly(1) + }) }) From 27d26b8a698402fee3e611692d4d47632bf9b79c Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Wed, 1 Jul 2020 18:07:35 +0100 Subject: [PATCH 02/17] test: refactor tests to avoid spies --- test/bind.js | 103 +++++++++++++++++++-------------------------------- 1 file changed, 38 insertions(+), 65 deletions(-) diff --git a/test/bind.js b/test/bind.js index 0a0c9a86..4d522d51 100644 --- a/test/bind.js +++ b/test/bind.js @@ -31,16 +31,11 @@ describe('bind', () => { const instance = document.createElement('bind-test-element') chai.spy.on(instance, 'foo') const el = document.createElement('div') - instance.querySelectorAll = () => [el] - el.closest = () => instance - el.getAttribute = () => 'click:bind-test-element#foo' - chai.spy.on(el, 'addEventListener') + el.setAttribute('data-action', 'click:bind-test-element#foo') + instance.appendChild(el) bind(instance) - expect(el.addEventListener).to.have.been.called.once.with('click') - const {calls} = el.addEventListener.__spy - const fn = calls[0][1] expect(instance.foo).to.have.not.been.called() - fn() + el.click() expect(instance.foo).to.have.been.called(1) }) @@ -48,33 +43,21 @@ describe('bind', () => { const instance = document.createElement('bind-test-element') chai.spy.on(instance, 'foo') const el = document.createElement('div') - instance.querySelectorAll = () => [el] - el.closest = () => instance - el.getAttribute = () => 'custom:event:bind-test-element#foo' - chai.spy.on(el, 'addEventListener') + el.setAttribute('data-action', 'custom:event:bind-test-element#foo') + instance.appendChild(el) bind(instance) - expect(el.addEventListener).to.have.been.called.once.with('custom:event') - const {calls} = el.addEventListener.__spy - const fn = calls[0][1] expect(instance.foo).to.have.not.been.called() - fn() + el.dispatchEvent(new CustomEvent('custom:event')) expect(instance.foo).to.have.been.called(1) }) it('binds events on the controller to itself', () => { const instance = document.createElement('bind-test-element') chai.spy.on(instance, 'foo') - instance.matches = () => true - instance.getAttribute = () => 'click:bind-test-element#foo' - instance.addEventListener = () => true - instance.querySelectorAll = () => [] - chai.spy.on(instance, 'addEventListener') + instance.setAttribute('data-action', 'click:bind-test-element#foo') bind(instance) - expect(instance.addEventListener).to.have.been.called.once.with('click') - const {calls} = instance.addEventListener.__spy - const fn = calls[0][1] expect(instance.foo).to.have.not.been.called() - fn() + instance.click() expect(instance.foo).to.have.been.called(1) }) @@ -82,56 +65,52 @@ describe('bind', () => { const instance = document.createElement('bind-test-element') chai.spy.on(instance, 'foo') const el = document.createElement('div') - instance.querySelectorAll = () => [el] - el.closest = () => null - el.getAttribute = () => 'click:bind-test-element#foo' - chai.spy.on(el, 'addEventListener') + el.getAttribute('data-action', 'click:bind-test-element#foo') + const container = document.createElement('div') + container.append(instance, el) bind(instance) - expect(el.addEventListener).to.have.not.been.called() + el.click() + expect(instance.foo).to.have.not.been.called() }) it('does not bind elements whose data-action does not match controller tagname', () => { const instance = document.createElement('bind-test-element') chai.spy.on(instance, 'foo') const el = document.createElement('div') - instance.querySelectorAll = () => [el] - el.closest = () => null - el.getAttribute = () => 'click:other-controller#foo' - chai.spy.on(el, 'addEventListener') + el.setAttribute('data-action', 'click:other-controller#foo') + instance.appendChild(el) bind(instance) - expect(el.addEventListener).to.have.not.been.called() + expect(instance.foo).to.have.not.been.called() + el.click() + expect(instance.foo).to.have.not.been.called() }) it('does not bind methods that dont exist', () => { const instance = document.createElement('bind-test-element') chai.spy.on(instance, 'foo') const el = document.createElement('div') - instance.querySelectorAll = () => [el] - el.closest = () => instance - el.getAttribute = () => 'click:bind-test-element#frob' - chai.spy.on(el, 'addEventListener') + el.setAttribute('data-action', 'click:bind-test-element#frob') + instance.appendChild(el) bind(instance) - expect(el.addEventListener).to.have.not.been.called() + el.click() + expect(instance.foo).to.have.not.been.called() }) it('can bind multiple event types', () => { const instance = document.createElement('bind-test-element') chai.spy.on(instance, 'foo') const el = document.createElement('div') - instance.querySelectorAll = () => [el] - el.closest = () => instance - el.getAttribute = () => 'click:bind-test-element#foo submit:bind-test-element#foo' - chai.spy.on(el, 'addEventListener') + el.setAttribute('data-action', 'click:bind-test-element#foo submit:bind-test-element#foo') + instance.appendChild(el) bind(instance) - expect(el.addEventListener).to.have.been.called(2) - expect(el.addEventListener).to.be.first.called.with('click') - expect(el.addEventListener).to.be.second.called.with('submit') - const {calls} = el.addEventListener.__spy expect(instance.foo).to.have.not.been.called() - calls[0][1]('a') - expect(instance.foo).to.have.been.called.once.with('a') - calls[1][1]('b') - expect(instance.foo).to.have.been.called.twice.second.with('b') + el.dispatchEvent(new CustomEvent('click')) + expect(instance.foo).to.have.been.called.exactly(1) + el.dispatchEvent(new CustomEvent('submit')) + expect(instance.foo).to.have.been.called.exactly(2) + const calls = instance.foo.__spy.calls + expect(calls).to.have.nested.property('[0][0].type', 'click') + expect(calls).to.have.nested.property('[1][0].type', 'submit') }) it('can bind multiple elements to the same event', () => { @@ -139,21 +118,15 @@ describe('bind', () => { chai.spy.on(instance, 'foo') const el1 = document.createElement('div') const el2 = document.createElement('div') - instance.querySelectorAll = () => [el1, el2] - el1.closest = () => instance - el2.closest = () => instance - el1.getAttribute = () => 'click:bind-test-element#foo' - el2.getAttribute = () => 'submit:bind-test-element#foo' - chai.spy.on(el1, 'addEventListener') - chai.spy.on(el2, 'addEventListener') + el1.setAttribute('data-action', 'click:bind-test-element#foo') + el2.setAttribute('data-action', 'submit:bind-test-element#foo') + instance.append(el1, el2) bind(instance) - expect(el1.addEventListener).to.be.called.once.with('click') - expect(el2.addEventListener).to.be.called.once.with('submit') expect(instance.foo).to.have.not.been.called() - el1.addEventListener.__spy.calls[0][1]('a') - expect(instance.foo).to.have.been.called.once.with('a') - el2.addEventListener.__spy.calls[0][1]('b') - expect(instance.foo).to.have.been.called.twice.second.with('b') + el1.click() + expect(instance.foo).to.have.been.called.exactly(1) + el2.dispatchEvent(new CustomEvent('submit')) + expect(instance.foo).to.have.been.called.exactly(2) }) describe('listenForBind', () => { From 273a81b74d52003c4586631af8c7839d42f004d4 Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Mon, 20 Jul 2020 09:43:13 +0100 Subject: [PATCH 03/17] test: add bind test to ensure removing attribute doesnt fire bind --- test/bind.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/bind.js b/test/bind.js index 4d522d51..c5be3ad4 100644 --- a/test/bind.js +++ b/test/bind.js @@ -199,4 +199,19 @@ describe('bind', () => { instance.querySelector('button').click() expect(instance.foo).to.have.been.called.exactly(1) }) + + it('will not fire if the binding attribute is removed', () => { + const instance = document.createElement('bind-test-element') + chai.spy.on(instance, 'foo') + const el1 = document.createElement('div') + el1.setAttribute('data-action', 'click:bind-test-element#foo') + instance.appendChild(el1) + bind(instance) + expect(instance.foo).to.have.not.been.called() + el1.click() + expect(instance.foo).to.have.been.called.exactly(1) + el1.setAttribute('data-action', 'click:other-element#foo') + el1.click() + expect(instance.foo).to.have.been.called.exactly(1) + }) }) From 09f5ad5709595b2e0a3b9c3063359c37c6a70132 Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Mon, 20 Jul 2020 09:43:57 +0100 Subject: [PATCH 04/17] test: add bind test asserting attribute changes are accomodated --- test/bind.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/bind.js b/test/bind.js index c5be3ad4..b4e9f14f 100644 --- a/test/bind.js +++ b/test/bind.js @@ -214,4 +214,23 @@ describe('bind', () => { el1.click() expect(instance.foo).to.have.been.called.exactly(1) }) + + it('will rebind elements if the attribute changes', async function () { + const instance = document.createElement('bind-test-element') + chai.spy.on(instance, 'foo') + root.appendChild(instance) + const button = document.createElement('button') + button.setAttribute('data-action', 'submit:bind-test-element#foo') + instance.appendChild(button) + bind(instance) + listenForBind(root) + await waitForNextAnimationFrame() + button.click() + expect(instance.foo).to.have.been.called.exactly(0) + button.setAttribute('data-action', 'click:bind-test-element#foo') + await waitForNextAnimationFrame() + await waitForNextAnimationFrame() + button.click() + expect(instance.foo).to.have.been.called.exactly(1) + }) }) From 23bdd70a6e6e949c9c9a1dd403330b6227890350 Mon Sep 17 00:00:00 2001 From: Mu-An Chiou Date: Fri, 17 Jul 2020 17:48:53 -0400 Subject: [PATCH 05/17] Add test case for element with connectedcallback bindings --- test/bind.js | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/test/bind.js b/test/bind.js index b4e9f14f..e97690a4 100644 --- a/test/bind.js +++ b/test/bind.js @@ -162,7 +162,7 @@ describe('bind', () => { expect(instance.foo).to.have.been.called.exactly(0) }) - it('will not re-bind elements that havent already had `bind()` called', async function () { + it('will not bind elements that havent already had `bind()` called', async function () { customElements.define('bind-test-not-element', class BindTestNotController extends HTMLElement {}) const instance = document.createElement('bind-test-not-element') chai.spy.on(instance, 'foo') @@ -178,6 +178,29 @@ describe('bind', () => { button.click() expect(instance.foo).to.have.been.called.exactly(0) }) + + it.only('will not re-bind elements that just had `bind()` called', async function () { + customElements.define( + 'bind-test-not-element', + class BindTestNotController extends HTMLElement { + connectedCallback() { + bind(this) + } + } + ) + const instance = document.createElement('bind-test-not-element') + chai.spy.on(instance, 'foo') + listenForBind(root) + const button = document.createElement('button') + button.setAttribute('data-action', 'click:bind-test-not-element#foo') + instance.appendChild(button) + root.appendChild(instance) + // wait for processQueue + await waitForNextAnimationFrame() + await waitForNextAnimationFrame() + button.click() + expect(instance.foo).to.have.been.called.exactly(1) + }) }) it('re-binds actions deeply in the HTML', async function () { From 9ef418ac4fbb8294c2bf06957718b4956ad71863 Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Mon, 20 Jul 2020 09:44:47 +0100 Subject: [PATCH 06/17] test: remove it.only --- test/bind.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/bind.js b/test/bind.js index e97690a4..2fa9cdf9 100644 --- a/test/bind.js +++ b/test/bind.js @@ -179,7 +179,7 @@ describe('bind', () => { expect(instance.foo).to.have.been.called.exactly(0) }) - it.only('will not re-bind elements that just had `bind()` called', async function () { + it('will not re-bind elements that just had `bind()` called', async function () { customElements.define( 'bind-test-not-element', class BindTestNotController extends HTMLElement { From 999f821dd646be3f89427a70ffc9411daf55f1ca Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Mon, 20 Jul 2020 11:51:01 +0100 Subject: [PATCH 07/17] test: remove useless test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kristján Oddsson --- test/bind.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/test/bind.js b/test/bind.js index 2fa9cdf9..65fac5b2 100644 --- a/test/bind.js +++ b/test/bind.js @@ -20,13 +20,6 @@ describe('bind', () => { root.remove() }) - it('queries for Elements matching data-action*="tagname"', () => { - const instance = document.createElement('bind-test-element') - chai.spy.on(instance, 'querySelectorAll', () => []) - bind(instance) - expect(instance.querySelectorAll).to.have.been.called.once.with.exactly('[data-action*=":bind-test-element#"]') - }) - it('binds events on elements based on their data-action attribute', () => { const instance = document.createElement('bind-test-element') chai.spy.on(instance, 'foo') From 6b1c723e3abe3b1872fbfca99ffb35e8fecf4b5e Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Mon, 20 Jul 2020 11:51:32 +0100 Subject: [PATCH 08/17] test: avoid re-defining custom elements in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kristján Oddsson --- test/bind.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/bind.js b/test/bind.js index 65fac5b2..1e094f4d 100644 --- a/test/bind.js +++ b/test/bind.js @@ -174,18 +174,18 @@ describe('bind', () => { it('will not re-bind elements that just had `bind()` called', async function () { customElements.define( - 'bind-test-not-element', + 'bind-test-not-rebind-element', class BindTestNotController extends HTMLElement { connectedCallback() { bind(this) } } ) - const instance = document.createElement('bind-test-not-element') + const instance = document.createElement('bind-test-not-rebind-element') chai.spy.on(instance, 'foo') listenForBind(root) const button = document.createElement('button') - button.setAttribute('data-action', 'click:bind-test-not-element#foo') + button.setAttribute('data-action', 'click:bind-test-not-rebind-element#foo') instance.appendChild(button) root.appendChild(instance) // wait for processQueue From 90a6b75536021a490866a260699bac71ebaccd91 Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Mon, 20 Jul 2020 11:53:16 +0100 Subject: [PATCH 09/17] refactor: ground up rewrite of bind/listenForBind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a rewrite from first-principles of `bind` and `listenForBind` to balance readability and performance, in light of the current requirements (as encoded by tests). Co-authored-by: Kristján Oddsson --- src/bind.ts | 159 +++++++++++++++++++++++++--------------------------- 1 file changed, 77 insertions(+), 82 deletions(-) diff --git a/src/bind.ts b/src/bind.ts index ef0d7871..e3f3327d 100644 --- a/src/bind.ts +++ b/src/bind.ts @@ -1,66 +1,12 @@ -const bound = new Set() +const controllers = new Set() + /* * Bind `[data-action]` elements from the DOM to their actions. * */ export function bind(controller: HTMLElement): void { - const tag = controller.tagName.toLowerCase() - bound.add(tag) - const actionAttributeMatcher = `[data-action*=":${tag}#"]` - - for (const el of controller.querySelectorAll(actionAttributeMatcher)) { - // Ignore nested elements - if (el.closest(tag) !== controller) continue - bindActionsToController(controller, el) - } - - // Also bind the controller to itself - if (controller.matches(actionAttributeMatcher)) { - bindActionsToController(controller, controller) - } -} - -// Match the pattern of `eventName:constructor#method`. -function* getActions(el: Element): Generator<[string, string, string]> { - for (const binding of (el.getAttribute('data-action') || '').split(' ')) { - const [rest, methodName] = binding.split('#') - if (!methodName) continue - - // eventName may contain `:` so account for that - // by splitting by the last instance of `:` - const colonIndex = rest.lastIndexOf(':') - if (colonIndex < 0) continue - - yield [rest.slice(0, colonIndex), rest.slice(colonIndex + 1), methodName] - } -} - -function bindActionToController(controller: HTMLElement, el: Element, methodName: string, eventName: string) { - // Check the `method` is present on the prototype - const methodDescriptor = - Object.getOwnPropertyDescriptor(controller, methodName) || - Object.getOwnPropertyDescriptor(Object.getPrototypeOf(controller), methodName) - if (methodDescriptor && typeof methodDescriptor.value == 'function') { - el.addEventListener(eventName, (event: Event) => { - methodDescriptor.value.call(controller, event) - }) - } -} - -// Bind the data-action attribute of a single element to the controller -function bindActionsToController(controller: HTMLElement, el: Element) { - const tag = controller.tagName.toLowerCase() - - for (const [eventName, tagName, methodName] of getActions(el)) { - if (tagName === tag) { - bindActionToController(controller, el, methodName, eventName) - } - } -} - -interface Subscription { - closed: boolean - unsubscribe(): void + controllers.add(controller.tagName.toLowerCase()) + findElementsToBind(controller) } /** @@ -70,26 +16,22 @@ interface Subscription { * This returns a Subscription object which you can call `unsubscribe()` on to * stop further live updates. */ -export function listenForBind(el: Node = document, batchSize = 30): Subscription { +export function listenForBind(el: Node = document): Subscription { let closed = false - const observer = new MutationObserver(mutations => { - const queue = new Set() for (const mutation of mutations) { - if (mutation.type === 'childList' && mutation.addedNodes.length) { + if (mutation.type === 'attributes' && mutation.target instanceof Element) { + bindActions(mutation.target) + } else if (mutation.type === 'childList' && mutation.addedNodes.length) { for (const node of mutation.addedNodes) { - if (!(node instanceof Element)) continue - if (node.hasAttribute('data-action')) { - queue.add(node) + if (node instanceof Element) { + findElementsToBind(node) } } } } - if (queue.size) requestAnimationFrame(() => processQueue(queue, batchSize)) }) - - observer.observe(el, {childList: true, subtree: true}) - + observer.observe(el, {childList: true, subtree: true, attributes: true, attributeFilter: ['data-action']}) return { get closed() { return closed @@ -101,22 +43,75 @@ export function listenForBind(el: Node = document, batchSize = 30): Subscription } } -function processQueue(queue: Set, batchSize: number) { - let counter = batchSize - for (const el of queue) { - for (const [eventName, controllerTag, methodName] of getActions(el)) { - if (!bound.has(controllerTag)) continue - const controller = el.closest(controllerTag) - if (!(controller instanceof HTMLElement)) continue +interface Subscription { + closed: boolean + unsubscribe(): void +} - bindActionToController(controller, el, methodName, eventName) +function findElementsToBind(root: Element) { + for (const el of root.querySelectorAll('[data-action]')) { + bindActions(el) + } + // Also bind the controller to itself + if (root.hasAttribute('data-action')) { + bindActions(root) + } +} + +function getActionEventName(action: string): string { + return action.slice(0, action.lastIndexOf(':')) +} + +function getActionControllerName(action: string): string { + return action.slice(action.lastIndexOf(':') + 1, action.lastIndexOf('#')) +} + +function getActionMethodName(action: string): string { + return action.slice(action.lastIndexOf('#') + 1) +} + +// ControllerEventHandler is a global event handler that dispatches events to +// controllers. We use a global event handler over bindings functions because +// this is far more performant; creating functions for each `addEventListener` +// would be very costly for CPU performance (and memory), while registering a +// single handler for every event keeps things relatively performant. +const ControllerEventHandler = { + handleEvent(event: Event) { + const el = event.currentTarget + if (!(el instanceof Element)) return + for (const action of (el.getAttribute('data-action') || '').split(' ')) { + // We want to dispatch this event, only to the subscribers; we filter by + // event.type to find which actions should fire + const eventType = getActionEventName(action) + if (event.type !== eventType) continue + // We need to find the closest controller to dispatch the event to. + const tagName = getActionControllerName(action) + // The controller should be "well known" in that `bind()` should have + // been called on it. + if (!controllers.has(tagName)) continue + const controller = el.closest(tagName) as Element & Record unknown> + if (!controller) continue + // Finally we need to get the right method to call on the controller. + // The method also needs to exist! + const method = getActionMethodName(action) + if (typeof controller[method] === 'function') { + controller[method](event) + } } - queue.delete(el) + } +} - counter -= 1 - if (counter === 0) break +const bindings = new WeakMap>() +function bindActions(el: Element) { + if (!bindings.has(el)) { + bindings.set(el, new Set()) } - if (queue.size !== 0) { - requestAnimationFrame(() => processQueue(queue, batchSize)) + const elementBindings = bindings.get(el)! + for (const action of (el.getAttribute('data-action') || '').split(' ')) { + const event = getActionEventName(action) + if (!elementBindings.has(event)) { + el.addEventListener(event, ControllerEventHandler) + } + elementBindings.add(event) } } From 795633b9f53af035b0a8abb39feef1cfee7df770 Mon Sep 17 00:00:00 2001 From: David Graham Date: Mon, 20 Jul 2020 10:24:00 -0600 Subject: [PATCH 10/17] Promote handleEvent to top-level function Co-authored-by: Keith Cirkel --- src/bind.ts | 44 +++++++++++++++++++++----------------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/src/bind.ts b/src/bind.ts index e3f3327d..9e6e36fa 100644 --- a/src/bind.ts +++ b/src/bind.ts @@ -75,28 +75,26 @@ function getActionMethodName(action: string): string { // this is far more performant; creating functions for each `addEventListener` // would be very costly for CPU performance (and memory), while registering a // single handler for every event keeps things relatively performant. -const ControllerEventHandler = { - handleEvent(event: Event) { - const el = event.currentTarget - if (!(el instanceof Element)) return - for (const action of (el.getAttribute('data-action') || '').split(' ')) { - // We want to dispatch this event, only to the subscribers; we filter by - // event.type to find which actions should fire - const eventType = getActionEventName(action) - if (event.type !== eventType) continue - // We need to find the closest controller to dispatch the event to. - const tagName = getActionControllerName(action) - // The controller should be "well known" in that `bind()` should have - // been called on it. - if (!controllers.has(tagName)) continue - const controller = el.closest(tagName) as Element & Record unknown> - if (!controller) continue - // Finally we need to get the right method to call on the controller. - // The method also needs to exist! - const method = getActionMethodName(action) - if (typeof controller[method] === 'function') { - controller[method](event) - } +function handleEvent(event: Event) { + const el = event.currentTarget + if (!(el instanceof Element)) return + for (const action of (el.getAttribute('data-action') || '').split(' ')) { + // We want to dispatch this event, only to the subscribers; we filter by + // event.type to find which actions should fire + const eventType = getActionEventName(action) + if (event.type !== eventType) continue + // We need to find the closest controller to dispatch the event to. + const tagName = getActionControllerName(action) + // The controller should be "well known" in that `bind()` should have + // been called on it. + if (!controllers.has(tagName)) continue + const controller = el.closest(tagName) as Element & Record unknown> + if (!controller) continue + // Finally we need to get the right method to call on the controller. + // The method also needs to exist! + const method = getActionMethodName(action) + if (typeof controller[method] === 'function') { + controller[method](event) } } } @@ -110,7 +108,7 @@ function bindActions(el: Element) { for (const action of (el.getAttribute('data-action') || '').split(' ')) { const event = getActionEventName(action) if (!elementBindings.has(event)) { - el.addEventListener(event, ControllerEventHandler) + el.addEventListener(event, handleEvent) } elementBindings.add(event) } From cb3bbec8939734a7a7c67ef370bed85b04a77621 Mon Sep 17 00:00:00 2001 From: David Graham Date: Mon, 20 Jul 2020 10:26:21 -0600 Subject: [PATCH 11/17] Remove event name tracking The addEventListener function binds only once based on the listener function's identity. We're passing `handleEvent` rather than a function closure so we can invoke addEventListener multiple times. Co-authored-by: Keith Cirkel --- src/bind.ts | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/src/bind.ts b/src/bind.ts index 9e6e36fa..750fe468 100644 --- a/src/bind.ts +++ b/src/bind.ts @@ -99,17 +99,8 @@ function handleEvent(event: Event) { } } -const bindings = new WeakMap>() function bindActions(el: Element) { - if (!bindings.has(el)) { - bindings.set(el, new Set()) - } - const elementBindings = bindings.get(el)! for (const action of (el.getAttribute('data-action') || '').split(' ')) { - const event = getActionEventName(action) - if (!elementBindings.has(event)) { - el.addEventListener(event, handleEvent) - } - elementBindings.add(event) + el.addEventListener(getActionEventName(action), handleEvent) } } From cc5c5834ba91c25ee2ec4d3ec48758156698e02d Mon Sep 17 00:00:00 2001 From: David Graham Date: Mon, 20 Jul 2020 10:29:31 -0600 Subject: [PATCH 12/17] findElementsToBind -> bindElements Co-authored-by: Keith Cirkel --- src/bind.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/bind.ts b/src/bind.ts index 750fe468..1355a888 100644 --- a/src/bind.ts +++ b/src/bind.ts @@ -6,7 +6,7 @@ const controllers = new Set() */ export function bind(controller: HTMLElement): void { controllers.add(controller.tagName.toLowerCase()) - findElementsToBind(controller) + bindElements(controller) } /** @@ -25,7 +25,7 @@ export function listenForBind(el: Node = document): Subscription { } else if (mutation.type === 'childList' && mutation.addedNodes.length) { for (const node of mutation.addedNodes) { if (node instanceof Element) { - findElementsToBind(node) + bindElements(node) } } } @@ -48,7 +48,7 @@ interface Subscription { unsubscribe(): void } -function findElementsToBind(root: Element) { +function bindElements(root: Element) { for (const el of root.querySelectorAll('[data-action]')) { bindActions(el) } From dcfa7feb5c5b6961f813c761c8cb0dc217c9cc62 Mon Sep 17 00:00:00 2001 From: David Graham Date: Mon, 20 Jul 2020 10:54:49 -0600 Subject: [PATCH 13/17] Clarify comments Co-authored-by: Keith Cirkel --- src/bind.ts | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/src/bind.ts b/src/bind.ts index 1355a888..2585be7b 100644 --- a/src/bind.ts +++ b/src/bind.ts @@ -70,28 +70,17 @@ function getActionMethodName(action: string): string { return action.slice(action.lastIndexOf('#') + 1) } -// ControllerEventHandler is a global event handler that dispatches events to -// controllers. We use a global event handler over bindings functions because -// this is far more performant; creating functions for each `addEventListener` -// would be very costly for CPU performance (and memory), while registering a -// single handler for every event keeps things relatively performant. +// Bind a single function to all events to avoid anonymous closure performance penalty. function handleEvent(event: Event) { const el = event.currentTarget if (!(el instanceof Element)) return for (const action of (el.getAttribute('data-action') || '').split(' ')) { - // We want to dispatch this event, only to the subscribers; we filter by - // event.type to find which actions should fire - const eventType = getActionEventName(action) - if (event.type !== eventType) continue - // We need to find the closest controller to dispatch the event to. + if (event.type !== getActionEventName(action)) continue const tagName = getActionControllerName(action) - // The controller should be "well known" in that `bind()` should have - // been called on it. + // Dispatch only to Catalyst elements. if (!controllers.has(tagName)) continue const controller = el.closest(tagName) as Element & Record unknown> if (!controller) continue - // Finally we need to get the right method to call on the controller. - // The method also needs to exist! const method = getActionMethodName(action) if (typeof controller[method] === 'function') { controller[method](event) From e34a50fa38db491151c89aecb05535ebe4230a60 Mon Sep 17 00:00:00 2001 From: David Graham Date: Mon, 20 Jul 2020 11:07:08 -0600 Subject: [PATCH 14/17] Introduce Binding type Co-authored-by: Keith Cirkel --- src/bind.ts | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/src/bind.ts b/src/bind.ts index 2585be7b..174f80bf 100644 --- a/src/bind.ts +++ b/src/bind.ts @@ -58,38 +58,35 @@ function bindElements(root: Element) { } } -function getActionEventName(action: string): string { - return action.slice(0, action.lastIndexOf(':')) -} - -function getActionControllerName(action: string): string { - return action.slice(action.lastIndexOf(':') + 1, action.lastIndexOf('#')) -} - -function getActionMethodName(action: string): string { - return action.slice(action.lastIndexOf('#') + 1) -} - // Bind a single function to all events to avoid anonymous closure performance penalty. function handleEvent(event: Event) { const el = event.currentTarget if (!(el instanceof Element)) return - for (const action of (el.getAttribute('data-action') || '').split(' ')) { - if (event.type !== getActionEventName(action)) continue - const tagName = getActionControllerName(action) + for (const binding of bindings(el)) { // Dispatch only to Catalyst elements. - if (!controllers.has(tagName)) continue - const controller = el.closest(tagName) as Element & Record unknown> + if (event.type !== binding.type || !controllers.has(binding.tag)) continue + const controller = el.closest(binding.tag) as Element & Record unknown> if (!controller) continue - const method = getActionMethodName(action) - if (typeof controller[method] === 'function') { - controller[method](event) + if (typeof controller[binding.method] === 'function') { + controller[binding.method](event) } } } +type Binding = {type: string; tag: string; method: string} +function bindings(el: Element): Binding[] { + return (el.getAttribute('data-action') || '').split(' ').map(action => { + const eventSep = action.lastIndexOf(':') + const methodSep = action.lastIndexOf('#') + const type = action.slice(0, eventSep) + const tag = action.slice(eventSep + 1, methodSep) + const method = action.slice(methodSep + 1) + return {type, tag, method} + }) +} + function bindActions(el: Element) { - for (const action of (el.getAttribute('data-action') || '').split(' ')) { - el.addEventListener(getActionEventName(action), handleEvent) + for (const binding of bindings(el)) { + el.addEventListener(binding.type, handleEvent) } } From a1b708b7018b00253a2b0f326f57e6cc8e0119f7 Mon Sep 17 00:00:00 2001 From: David Graham Date: Mon, 20 Jul 2020 11:08:42 -0600 Subject: [PATCH 15/17] Replace continue with if statements Co-authored-by: Keith Cirkel --- src/bind.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/bind.ts b/src/bind.ts index 174f80bf..5005685c 100644 --- a/src/bind.ts +++ b/src/bind.ts @@ -63,12 +63,11 @@ function handleEvent(event: Event) { const el = event.currentTarget if (!(el instanceof Element)) return for (const binding of bindings(el)) { - // Dispatch only to Catalyst elements. - if (event.type !== binding.type || !controllers.has(binding.tag)) continue - const controller = el.closest(binding.tag) as Element & Record unknown> - if (!controller) continue - if (typeof controller[binding.method] === 'function') { - controller[binding.method](event) + if (event.type === binding.type && controllers.has(binding.tag)) { + const controller = el.closest(binding.tag) as Element & Record unknown> + if (controller && typeof controller[binding.method] === 'function') { + controller[binding.method](event) + } } } } From ccccb634841501fe5b6f2e8a6a2fc177fddf3693 Mon Sep 17 00:00:00 2001 From: David Graham Date: Mon, 20 Jul 2020 11:10:54 -0600 Subject: [PATCH 16/17] Assert event target is Element The handleEvent function is bound only to Element targets. Co-authored-by: Keith Cirkel --- src/bind.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/bind.ts b/src/bind.ts index 5005685c..85282237 100644 --- a/src/bind.ts +++ b/src/bind.ts @@ -60,8 +60,7 @@ function bindElements(root: Element) { // Bind a single function to all events to avoid anonymous closure performance penalty. function handleEvent(event: Event) { - const el = event.currentTarget - if (!(el instanceof Element)) return + const el = event.currentTarget as Element for (const binding of bindings(el)) { if (event.type === binding.type && controllers.has(binding.tag)) { const controller = el.closest(binding.tag) as Element & Record unknown> From b45d3bcbc92c2f1a7b66a62eeaccd953edf168a2 Mon Sep 17 00:00:00 2001 From: David Graham Date: Mon, 20 Jul 2020 11:32:47 -0600 Subject: [PATCH 17/17] Use generator function Co-authored-by: Keith Cirkel --- src/bind.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/bind.ts b/src/bind.ts index 85282237..764d08ea 100644 --- a/src/bind.ts +++ b/src/bind.ts @@ -72,15 +72,15 @@ function handleEvent(event: Event) { } type Binding = {type: string; tag: string; method: string} -function bindings(el: Element): Binding[] { - return (el.getAttribute('data-action') || '').split(' ').map(action => { +function* bindings(el: Element): Iterable { + for (const action of (el.getAttribute('data-action') || '').split(' ')) { const eventSep = action.lastIndexOf(':') const methodSep = action.lastIndexOf('#') const type = action.slice(0, eventSep) const tag = action.slice(eventSep + 1, methodSep) const method = action.slice(methodSep + 1) - return {type, tag, method} - }) + yield {type, tag, method} + } } function bindActions(el: Element) {