From 3aa95d96d45777670e9144d0e7bb3b35aa8ce2d1 Mon Sep 17 00:00:00 2001 From: Will Binns-Smith Date: Mon, 21 Aug 2017 14:47:22 -0700 Subject: [PATCH 1/6] [Commands] Add support for rich listener objects with first-class metadata This adds support for listener objects which, in addition to the existing callback listeners, can optionally provide a displayName ahead of time to avoid a potentially awkward humanized displayName. --- spec/command-registry-spec.js | 94 ++++++++++++++++++++++++++++------- src/command-registry.js | 84 ++++++++++++++++++------------- 2 files changed, 126 insertions(+), 52 deletions(-) diff --git a/spec/command-registry-spec.js b/spec/command-registry-spec.js index c88097efd..4c923be87 100644 --- a/spec/command-registry-spec.js +++ b/spec/command-registry-spec.js @@ -224,32 +224,35 @@ describe("CommandRegistry", () => { expect(addError.message).toContain(badSelector); }); - it("throws an error when called with a non-function callback and selector target", () => { + it("throws an error when called with a null callback and selector target", () => { const badCallback = null; - let addError = null; - try { + expect(() => { registry.add('.selector', 'foo:bar', badCallback); - } catch (error) { - addError = error; - } - expect(addError.message).toContain("Can't register a command with non-function callback."); + }).toThrow(new Error('Cannot register a command with a null listener.')); }); - it("throws an error when called with an non-function callback and object target", () => { + it("throws an error when called with an null callback and object target", () => { const badCallback = null; - let addError = null; - try { + expect(() => { registry.add(document.body, 'foo:bar', badCallback); - } catch (error) { - addError = error; - } - expect(addError.message).toContain("Can't register a command with non-function callback."); + }).toThrow(new Error('Cannot register a command with a null listener.')); + }); + + it("throws an error when called with an object listener without a handleEvent method", () => { + const badListener = { + title: 'a listener without a handleEvent callback', + description: 'this should throw an error' + }; + + expect(() => { + registry.add(document.body, 'foo:bar', badListener); + }).toThrow(new Error('Listener must be a callback function or an object with a handleEvent method.')); }); }); - describe("::findCommands({target})", () => + describe("::findCommands({target})", () => { it("returns commands that can be invoked on the target or its ancestors", () => { registry.add('.parent', 'namespace:command-1', () => {}); registry.add('.child', 'namespace:command-2', () => {}); @@ -268,8 +271,65 @@ describe("CommandRegistry", () => { {name: 'namespace:command-2', displayName: 'Namespace: Command 2'}, {name: 'namespace:command-1', displayName: 'Namespace: Command 1'} ]); - }) -); + }); + + it("returns commands with manual displayNames if set in the listener", () => { + registry.add('.grandchild', 'namespace:command-1', () => {}); + registry.add('.grandchild', 'namespace:command-2', { + displayName: 'Custom Command 2', + metadata: { + some: 'other', + object: 'data' + }, + handleEvent() {} + }); + registry.add('.grandchild', 'namespace:command-3', () => {}); + + const commands = registry.findCommands({target: grandchild}); + expect(commands).toEqual([ + { + displayName: 'Namespace: Command 1', + name: 'namespace:command-1' + }, + { + displayName: 'Custom Command 2', + metadata: { + some : 'other', + object : 'data' + }, + name: 'namespace:command-2' + }, + { + displayName: 'Namespace: Command 3', + name: 'namespace:command-3' + } + ]); + }); + + it("ignores a `name` property if passed in registration object", () => { + registry.add('.grandchild', 'namespace:command-2', { + name: 'some:other:commandname', + displayName: 'Custom Command 2', + metadata: { + some: 'other', + object: 'data' + }, + handleEvent() {} + }); + + const commands = registry.findCommands({target: grandchild}); + expect(commands).toEqual([ + { + displayName: 'Custom Command 2', + metadata: { + some : 'other', + object : 'data' + }, + name: 'namespace:command-2' + } + ]); + }); + }); describe("::dispatch(target, commandName)", () => { it("simulates invocation of the given command ", () => { diff --git a/src/command-registry.js b/src/command-registry.js index 0d86e723c..7a3be1028 100644 --- a/src/command-registry.js +++ b/src/command-registry.js @@ -109,72 +109,74 @@ module.exports = class CommandRegistry { // // Returns a {Disposable} on which `.dispose()` can be called to remove the // added command handler(s). - add (target, commandName, callback, throwOnInvalidSelector = true) { + add (target, commandName, listener, throwOnInvalidSelector = true) { if (typeof commandName === 'object') { const commands = commandName - throwOnInvalidSelector = callback - const disposable = new CompositeDisposable() + throwOnInvalidSelector = listener + const disposable = new CompositeDisposable for (commandName in commands) { - callback = commands[commandName] - disposable.add( - this.add(target, commandName, callback, throwOnInvalidSelector) - ) + listener = commands[commandName] + disposable.add(this.add(target, commandName, listener, throwOnInvalidSelector)) } return disposable } - if (typeof callback !== 'function') { - throw new Error("Can't register a command with non-function callback.") + if ((listener === null) || (listener === undefined)) { + throw new Error('Cannot register a command with a null listener.') + } + + if ((typeof listener !== 'function') && (typeof listener.handleEvent !== 'function')) { + throw new Error('Listener must be a callback function or an object with a handleEvent method.') } if (typeof target === 'string') { if (throwOnInvalidSelector) { validateSelector(target) } - return this.addSelectorBasedListener(target, commandName, callback) + return this.addSelectorBasedListener(target, commandName, listener) } else { - return this.addInlineListener(target, commandName, callback) + return this.addInlineListener(target, commandName, listener) } } - addSelectorBasedListener (selector, commandName, callback) { + addSelectorBasedListener (selector, commandName, listener) { if (this.selectorBasedListenersByCommandName[commandName] == null) { this.selectorBasedListenersByCommandName[commandName] = [] } const listenersForCommand = this.selectorBasedListenersByCommandName[commandName] - const listener = new SelectorBasedListener(selector, callback) - listenersForCommand.push(listener) + const selectorListener = new SelectorBasedListener(selector, commandFromListener(listener)) + listenersForCommand.push(selectorListener) this.commandRegistered(commandName) return new Disposable(() => { - listenersForCommand.splice(listenersForCommand.indexOf(listener), 1) + listenersForCommand.splice(listenersForCommand.indexOf(selectorListener), 1) if (listenersForCommand.length === 0) { - return delete this.selectorBasedListenersByCommandName[commandName] + delete this.selectorBasedListenersByCommandName[commandName] } }) } - addInlineListener (element, commandName, callback) { + addInlineListener (element, commandName, listener) { + let listenersForElement if (this.inlineListenersByCommandName[commandName] == null) { - this.inlineListenersByCommandName[commandName] = new WeakMap() + this.inlineListenersByCommandName[commandName] = new WeakMap } const listenersForCommand = this.inlineListenersByCommandName[commandName] - let listenersForElement = listenersForCommand.get(element) - if (listenersForElement == null) { + if (!(listenersForElement = listenersForCommand.get(element))) { listenersForElement = [] listenersForCommand.set(element, listenersForElement) } - const listener = new InlineListener(callback) - listenersForElement.push(listener) + const inlineListener = new InlineListener(commandFromListener(listener)) + listenersForElement.push(inlineListener) this.commandRegistered(commandName) - return new Disposable(function () { - listenersForElement.splice(listenersForElement.indexOf(listener), 1) + return new Disposable(() => { + listenersForElement.splice(listenersForElement.indexOf(inlineListener), 1) if (listenersForElement.length === 0) { - return listenersForCommand.delete(element) + listenersForCommand.delete(element) } }) } @@ -198,7 +200,10 @@ module.exports = class CommandRegistry { listeners = this.inlineListenersByCommandName[name] if (listeners.has(currentTarget) && !commandNames.has(name)) { commandNames.add(name) - commands.push({ name, displayName: _.humanizeEventName(name) }) + // don't allow those with a command derived from an object to invoke its + // handler directly. rather, they should call ::dispatch a CustomEvent with + // its `name` property + commands.push(_.omit(listeners.get(currentTarget), 'handleEvent')) } } @@ -211,10 +216,7 @@ module.exports = class CommandRegistry { ) { if (!commandNames.has(commandName)) { commandNames.add(commandName) - commands.push({ - name: commandName, - displayName: _.humanizeEventName(commandName) - }) + commands.push(_.omit(listener, 'handleEvent')) } } } @@ -358,7 +360,7 @@ module.exports = class CommandRegistry { if (immediatePropagationStopped) { break } - listener.callback.call(currentTarget, dispatchedEvent) + listener.command.call(currentTarget, dispatchedEvent) } if (currentTarget === window) { @@ -384,9 +386,9 @@ module.exports = class CommandRegistry { } class SelectorBasedListener { - constructor (selector, callback) { + constructor (selector, command) { this.selector = selector - this.callback = callback + this.command = command this.specificity = calculateSpecificity(this.selector) this.sequenceNumber = SequenceCount++ } @@ -400,7 +402,19 @@ class SelectorBasedListener { } class InlineListener { - constructor (callback) { - this.callback = callback + constructor (command) { + this.command = command } } + +function commandFromListener (name, listener) { + return Object.assign( + {}, + listener, + { + name, + displayName: listener.displayName ? listener.displayName : _.humanizeEventName(name), + handleEvent: typeof listener === 'function' ? listener : listener.handleEvent, + } + ) +} From 9b995c68ec52a35c5d38315a22b1c995bbbd6e95 Mon Sep 17 00:00:00 2001 From: Will Binns-Smith Date: Mon, 21 Aug 2017 16:22:24 -0700 Subject: [PATCH 2/6] Normalize listeners at registration time; add tests for rich functions --- spec/command-registry-spec.js | 42 +++++++++++++++---------- src/command-registry.js | 58 ++++++++++++++++++++++------------- 2 files changed, 62 insertions(+), 38 deletions(-) diff --git a/spec/command-registry-spec.js b/spec/command-registry-spec.js index 4c923be87..969bd1a84 100644 --- a/spec/command-registry-spec.js +++ b/spec/command-registry-spec.js @@ -273,7 +273,7 @@ describe("CommandRegistry", () => { ]); }); - it("returns commands with manual displayNames if set in the listener", () => { + it("returns commands with arbitrary metadata if set in a listener object", () => { registry.add('.grandchild', 'namespace:command-1', () => {}); registry.add('.grandchild', 'namespace:command-2', { displayName: 'Custom Command 2', @@ -283,7 +283,15 @@ describe("CommandRegistry", () => { }, handleEvent() {} }); - registry.add('.grandchild', 'namespace:command-3', () => {}); + registry.add('.grandchild', 'namespace:command-3', { + name: 'some:other:incorrect:commandname', + displayName: 'Custom Command 3', + metadata: { + some: 'other', + object: 'data' + }, + handleEvent() {} + }); const commands = registry.findCommands({target: grandchild}); expect(commands).toEqual([ @@ -300,30 +308,32 @@ describe("CommandRegistry", () => { name: 'namespace:command-2' }, { - displayName: 'Namespace: Command 3', + displayName: 'Custom Command 3', + metadata: { + some : 'other', + object : 'data' + }, name: 'namespace:command-3' } ]); }); - it("ignores a `name` property if passed in registration object", () => { - registry.add('.grandchild', 'namespace:command-2', { - name: 'some:other:commandname', - displayName: 'Custom Command 2', - metadata: { - some: 'other', - object: 'data' - }, - handleEvent() {} - }); + it("returns commands with arbitrary metadata if set on a listener function", () => { + function listener () {} + listener.displayName = 'Custom Command 2' + listener.metadata = { + some: 'other', + object: 'data' + }; + registry.add('.grandchild', 'namespace:command-2', listener); const commands = registry.findCommands({target: grandchild}); expect(commands).toEqual([ { - displayName: 'Custom Command 2', + displayName : 'Custom Command 2', metadata: { - some : 'other', - object : 'data' + some: 'other', + object: 'data' }, name: 'namespace:command-2' } diff --git a/src/command-registry.js b/src/command-registry.js index 7a3be1028..182fb8f47 100644 --- a/src/command-registry.js +++ b/src/command-registry.js @@ -144,7 +144,7 @@ module.exports = class CommandRegistry { this.selectorBasedListenersByCommandName[commandName] = [] } const listenersForCommand = this.selectorBasedListenersByCommandName[commandName] - const selectorListener = new SelectorBasedListener(selector, commandFromListener(listener)) + const selectorListener = new SelectorBasedListener(selector, commandName, listener) listenersForCommand.push(selectorListener) this.commandRegistered(commandName) @@ -158,17 +158,17 @@ module.exports = class CommandRegistry { } addInlineListener (element, commandName, listener) { - let listenersForElement if (this.inlineListenersByCommandName[commandName] == null) { this.inlineListenersByCommandName[commandName] = new WeakMap } const listenersForCommand = this.inlineListenersByCommandName[commandName] - if (!(listenersForElement = listenersForCommand.get(element))) { + let listenersForElement = listenersForCommand.get(element) + if (!listenersForElement) { listenersForElement = [] listenersForCommand.set(element, listenersForElement) } - const inlineListener = new InlineListener(commandFromListener(listener)) + const inlineListener = new InlineListener(commandName, listener) listenersForElement.push(inlineListener) this.commandRegistered(commandName) @@ -200,23 +200,20 @@ module.exports = class CommandRegistry { listeners = this.inlineListenersByCommandName[name] if (listeners.has(currentTarget) && !commandNames.has(name)) { commandNames.add(name) - // don't allow those with a command derived from an object to invoke its - // handler directly. rather, they should call ::dispatch a CustomEvent with - // its `name` property - commands.push(_.omit(listeners.get(currentTarget), 'handleEvent')) + const targetListeners = listeners.get(currentTarget); + commands.push( + ...targetListeners.map(listener => listener.getPublicCommand()) + ); } } for (const commandName in this.selectorBasedListenersByCommandName) { listeners = this.selectorBasedListenersByCommandName[commandName] for (const listener of listeners) { - if ( - currentTarget.webkitMatchesSelector && - currentTarget.webkitMatchesSelector(listener.selector) - ) { + if (listener.matchesTarget(currentTarget)) { if (!commandNames.has(commandName)) { commandNames.add(commandName) - commands.push(_.omit(listener, 'handleEvent')) + commands.push(listener.getPublicCommand()) } } } @@ -341,9 +338,7 @@ module.exports = class CommandRegistry { if (currentTarget.webkitMatchesSelector != null) { const selectorBasedListeners = (this.selectorBasedListenersByCommandName[event.type] || []) - .filter(listener => - currentTarget.webkitMatchesSelector(listener.selector) - ) + .filter(listener => listener.matchesTarget(currentTarget)) .sort((a, b) => a.compare(b)) listeners = selectorBasedListeners.concat(listeners) } @@ -360,7 +355,7 @@ module.exports = class CommandRegistry { if (immediatePropagationStopped) { break } - listener.command.call(currentTarget, dispatchedEvent) + listener.command.handleEvent.call(currentTarget, dispatchedEvent) } if (currentTarget === window) { @@ -386,9 +381,9 @@ module.exports = class CommandRegistry { } class SelectorBasedListener { - constructor (selector, command) { + constructor (selector, commandName, listener) { this.selector = selector - this.command = command + this.command = commandFromListener(commandName, listener) this.specificity = calculateSpecificity(this.selector) this.sequenceNumber = SequenceCount++ } @@ -399,11 +394,23 @@ class SelectorBasedListener { this.sequenceNumber - other.sequenceNumber ) } + + getPublicCommand () { + return getPublicCommand.call(this) + } + + matchesTarget (target) { + return target.webkitMatchesSelector && target.webkitMatchesSelector(this.selector) + } } class InlineListener { - constructor (command) { - this.command = command + constructor (commandName, listener) { + this.command = commandFromListener(commandName, listener) + } + + getPublicCommand () { + return getPublicCommand.call(this) } } @@ -414,7 +421,14 @@ function commandFromListener (name, listener) { { name, displayName: listener.displayName ? listener.displayName : _.humanizeEventName(name), - handleEvent: typeof listener === 'function' ? listener : listener.handleEvent, + handleEvent: typeof listener === 'function' ? listener : listener.handleEvent } ) } + +// don't allow those with a command derived from an object to invoke its +// handler directly. rather, they should call ::dispatch a CustomEvent with +// its `name` property +function getPublicCommand() { + return _.omit(this.command, 'handleEvent') +} From b3a296e802050125074cfa074703e07ccd9677c6 Mon Sep 17 00:00:00 2001 From: Will Binns-Smith Date: Mon, 21 Aug 2017 22:18:51 -0700 Subject: [PATCH 3/6] Eagerly extract descriptor and callback --- src/command-registry.js | 73 ++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 30 deletions(-) diff --git a/src/command-registry.js b/src/command-registry.js index 182fb8f47..c7f1c338b 100644 --- a/src/command-registry.js +++ b/src/command-registry.js @@ -91,11 +91,22 @@ module.exports = class CommandRegistry { // DOM element, the command will be associated with just that element. // * `commandName` A {String} containing the name of a command you want to // handle such as `user:insert-date`. - // * `callback` A {Function} to call when the given command is invoked on an - // element matching the selector. It will be called with `this` referencing - // the matching DOM node. - // * `event` A standard DOM event instance. Call `stopPropagation` or - // `stopImmediatePropagation` to terminate bubbling early. + // * `listener` A listener which handles the event. Either A {Function} to + // call when the given command is invoked on an element matching the + // selector, or an {Object} with a `handleEvent` property which is such a + // function. + // + // It will be called with `this` referencing the matching DOM node. + // * `event` A standard DOM event instance. Call `stopPropagation` or + // `stopImmediatePropagation` to terminate bubbling early. + // + // Additionally, `listener` may have additional properties which are returned + // to those who query using `atom.commands.findCommands`, as well as several + // meaningful metadata properties: + // * `displayName`: Overrides any generated `displayName` that would + // otherwise be generated from the event name. + // * `description`: Used by consumers to display detailed information about + // the command. // // ## Arguments: Registering Multiple Commands // @@ -121,7 +132,12 @@ module.exports = class CommandRegistry { return disposable } - if ((listener === null) || (listener === undefined)) { + // type Listener = ?(e: CustomEvent) => void | ?{ + // displayName?: string, + // description?: string, + // handleEvent(e: CustomEvent): void, + // } + if (listener == null) { throw new Error('Cannot register a command with a null listener.') } @@ -186,7 +202,7 @@ module.exports = class CommandRegistry { // * `params` An {Object} containing one or more of the following keys: // * `target` A DOM node that is the hypothetical target of a given command. // - // Returns an {Array} of {Object}s containing the following keys: + // Returns an {Array} of `CommandDescriptor` {Object}s containing the following keys: // * `name` The name of the command. For example, `user:insert-date`. // * `displayName` The display name of the command. For example, // `User: Insert Date`. @@ -202,7 +218,7 @@ module.exports = class CommandRegistry { commandNames.add(name) const targetListeners = listeners.get(currentTarget); commands.push( - ...targetListeners.map(listener => listener.getPublicCommand()) + ...targetListeners.map(listener => listener.descriptor) ); } } @@ -213,7 +229,7 @@ module.exports = class CommandRegistry { if (listener.matchesTarget(currentTarget)) { if (!commandNames.has(commandName)) { commandNames.add(commandName) - commands.push(listener.getPublicCommand()) + commands.push(listener.descriptor) } } } @@ -355,7 +371,7 @@ module.exports = class CommandRegistry { if (immediatePropagationStopped) { break } - listener.command.handleEvent.call(currentTarget, dispatchedEvent) + listener.callback.call(currentTarget, dispatchedEvent) } if (currentTarget === window) { @@ -380,10 +396,15 @@ module.exports = class CommandRegistry { } } +// type Listener = { +// descriptor: CommandDescriptor, +// callback: (e: CustomEvent) => void, +// }; class SelectorBasedListener { constructor (selector, commandName, listener) { this.selector = selector - this.command = commandFromListener(commandName, listener) + this.callback = extractCallback(listener) + this.descriptor = extractDescriptor(commandName, listener) this.specificity = calculateSpecificity(this.selector) this.sequenceNumber = SequenceCount++ } @@ -395,40 +416,32 @@ class SelectorBasedListener { ) } - getPublicCommand () { - return getPublicCommand.call(this) - } - matchesTarget (target) { - return target.webkitMatchesSelector && target.webkitMatchesSelector(this.selector) + return target.webkitMatchesSelector && target.webkitMatchesSelector(this.selector) } } class InlineListener { constructor (commandName, listener) { - this.command = commandFromListener(commandName, listener) - } - - getPublicCommand () { - return getPublicCommand.call(this) + this.callback = extractCallback(listener); + this.descriptor = extractDescriptor(commandName, listener) } } -function commandFromListener (name, listener) { +// type CommandDescriptor = { +// name: string, +// displayName: string, +// }; +function extractDescriptor (name, listener) { return Object.assign( - {}, - listener, + _.omit(listener, 'handleEvent'), { name, displayName: listener.displayName ? listener.displayName : _.humanizeEventName(name), - handleEvent: typeof listener === 'function' ? listener : listener.handleEvent } ) } -// don't allow those with a command derived from an object to invoke its -// handler directly. rather, they should call ::dispatch a CustomEvent with -// its `name` property -function getPublicCommand() { - return _.omit(this.command, 'handleEvent') +function extractCallback (listener) { + return typeof listener === 'function' ? listener : listener.handleEvent; } From c916c9d81805945241309db492fa3d89b40c24d3 Mon Sep 17 00:00:00 2001 From: Will Binns-Smith Date: Mon, 21 Aug 2017 22:26:55 -0700 Subject: [PATCH 4/6] Better document the new api externally and internally --- src/command-registry.js | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/command-registry.js b/src/command-registry.js index c7f1c338b..c385e8b18 100644 --- a/src/command-registry.js +++ b/src/command-registry.js @@ -1,7 +1,5 @@ 'use strict' -/* global Event, CustomEvent */ - const { Emitter, Disposable, CompositeDisposable } = require('event-kit') const { calculateSpecificity, validateSelector } = require('clear-cut') const _ = require('underscore-plus') @@ -124,7 +122,7 @@ module.exports = class CommandRegistry { if (typeof commandName === 'object') { const commands = commandName throwOnInvalidSelector = listener - const disposable = new CompositeDisposable + const disposable = new CompositeDisposable() for (commandName in commands) { listener = commands[commandName] disposable.add(this.add(target, commandName, listener, throwOnInvalidSelector)) @@ -132,15 +130,15 @@ module.exports = class CommandRegistry { return disposable } - // type Listener = ?(e: CustomEvent) => void | ?{ - // displayName?: string, - // description?: string, - // handleEvent(e: CustomEvent): void, - // } if (listener == null) { throw new Error('Cannot register a command with a null listener.') } + // type Listener = ((e: CustomEvent) => void) | { + // displayName?: string, + // description?: string, + // handleEvent(e: CustomEvent): void, + // } if ((typeof listener !== 'function') && (typeof listener.handleEvent !== 'function')) { throw new Error('Listener must be a callback function or an object with a handleEvent method.') } @@ -175,7 +173,7 @@ module.exports = class CommandRegistry { addInlineListener (element, commandName, listener) { if (this.inlineListenersByCommandName[commandName] == null) { - this.inlineListenersByCommandName[commandName] = new WeakMap + this.inlineListenersByCommandName[commandName] = new WeakMap() } const listenersForCommand = this.inlineListenersByCommandName[commandName] @@ -206,6 +204,13 @@ module.exports = class CommandRegistry { // * `name` The name of the command. For example, `user:insert-date`. // * `displayName` The display name of the command. For example, // `User: Insert Date`. + // Additional metadata may also be present in the returned descriptor: + // * `description` a {String} describing the function of the command in more + // detail than the title + // * `tags` an {Array} of {String}s that describe keywords related to the + // command + // Any additional nonstandard metadata provided when the command was `add`ed + // may also be present in the returned descriptor. findCommands ({ target }) { const commandNames = new Set() const commands = [] @@ -216,10 +221,10 @@ module.exports = class CommandRegistry { listeners = this.inlineListenersByCommandName[name] if (listeners.has(currentTarget) && !commandNames.has(name)) { commandNames.add(name) - const targetListeners = listeners.get(currentTarget); + const targetListeners = listeners.get(currentTarget) commands.push( ...targetListeners.map(listener => listener.descriptor) - ); + ) } } @@ -423,7 +428,7 @@ class SelectorBasedListener { class InlineListener { constructor (commandName, listener) { - this.callback = extractCallback(listener); + this.callback = extractCallback(listener) this.descriptor = extractDescriptor(commandName, listener) } } @@ -437,11 +442,11 @@ function extractDescriptor (name, listener) { _.omit(listener, 'handleEvent'), { name, - displayName: listener.displayName ? listener.displayName : _.humanizeEventName(name), + displayName: listener.displayName ? listener.displayName : _.humanizeEventName(name) } ) } function extractCallback (listener) { - return typeof listener === 'function' ? listener : listener.handleEvent; + return typeof listener === 'function' ? listener : listener.handleEvent } From 2ac1d54557f901b82c513ebab3b92dce2845a4f6 Mon Sep 17 00:00:00 2001 From: Will Binns-Smith Date: Tue, 22 Aug 2017 14:32:32 -0700 Subject: [PATCH 5/6] handleEvent -> onDidDispatch --- spec/command-registry-spec.js | 10 +++++----- src/command-registry.js | 22 +++++++++++----------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/spec/command-registry-spec.js b/spec/command-registry-spec.js index 969bd1a84..5dbd8d14d 100644 --- a/spec/command-registry-spec.js +++ b/spec/command-registry-spec.js @@ -240,15 +240,15 @@ describe("CommandRegistry", () => { }).toThrow(new Error('Cannot register a command with a null listener.')); }); - it("throws an error when called with an object listener without a handleEvent method", () => { + it("throws an error when called with an object listener without a onDidDispatch method", () => { const badListener = { - title: 'a listener without a handleEvent callback', + title: 'a listener without a onDidDispatch callback', description: 'this should throw an error' }; expect(() => { registry.add(document.body, 'foo:bar', badListener); - }).toThrow(new Error('Listener must be a callback function or an object with a handleEvent method.')); + }).toThrow(new Error('Listener must be a callback function or an object with a onDidDispatch method.')); }); }); @@ -281,7 +281,7 @@ describe("CommandRegistry", () => { some: 'other', object: 'data' }, - handleEvent() {} + onDidDispatch() {} }); registry.add('.grandchild', 'namespace:command-3', { name: 'some:other:incorrect:commandname', @@ -290,7 +290,7 @@ describe("CommandRegistry", () => { some: 'other', object: 'data' }, - handleEvent() {} + onDidDispatch() {} }); const commands = registry.findCommands({target: grandchild}); diff --git a/src/command-registry.js b/src/command-registry.js index c385e8b18..5e4553603 100644 --- a/src/command-registry.js +++ b/src/command-registry.js @@ -91,7 +91,7 @@ module.exports = class CommandRegistry { // handle such as `user:insert-date`. // * `listener` A listener which handles the event. Either A {Function} to // call when the given command is invoked on an element matching the - // selector, or an {Object} with a `handleEvent` property which is such a + // selector, or an {Object} with a `onDidDispatch` property which is such a // function. // // It will be called with `this` referencing the matching DOM node. @@ -137,10 +137,10 @@ module.exports = class CommandRegistry { // type Listener = ((e: CustomEvent) => void) | { // displayName?: string, // description?: string, - // handleEvent(e: CustomEvent): void, + // onDidDispatch(e: CustomEvent): void, // } - if ((typeof listener !== 'function') && (typeof listener.handleEvent !== 'function')) { - throw new Error('Listener must be a callback function or an object with a handleEvent method.') + if ((typeof listener !== 'function') && (typeof listener.onDidDispatch !== 'function')) { + throw new Error('Listener must be a callback function or an object with a onDidDispatch method.') } if (typeof target === 'string') { @@ -376,7 +376,7 @@ module.exports = class CommandRegistry { if (immediatePropagationStopped) { break } - listener.callback.call(currentTarget, dispatchedEvent) + listener.onDidDispatch.call(currentTarget, dispatchedEvent) } if (currentTarget === window) { @@ -403,12 +403,12 @@ module.exports = class CommandRegistry { // type Listener = { // descriptor: CommandDescriptor, -// callback: (e: CustomEvent) => void, +// extractOnDidDispatch: (e: CustomEvent) => void, // }; class SelectorBasedListener { constructor (selector, commandName, listener) { this.selector = selector - this.callback = extractCallback(listener) + this.onDidDispatch = extractOnDidDispatch(listener) this.descriptor = extractDescriptor(commandName, listener) this.specificity = calculateSpecificity(this.selector) this.sequenceNumber = SequenceCount++ @@ -428,7 +428,7 @@ class SelectorBasedListener { class InlineListener { constructor (commandName, listener) { - this.callback = extractCallback(listener) + this.onDidDispatch = extractOnDidDispatch(listener) this.descriptor = extractDescriptor(commandName, listener) } } @@ -439,7 +439,7 @@ class InlineListener { // }; function extractDescriptor (name, listener) { return Object.assign( - _.omit(listener, 'handleEvent'), + _.omit(listener, 'onDidDispatch'), { name, displayName: listener.displayName ? listener.displayName : _.humanizeEventName(name) @@ -447,6 +447,6 @@ function extractDescriptor (name, listener) { ) } -function extractCallback (listener) { - return typeof listener === 'function' ? listener : listener.handleEvent +function extractOnDidDispatch (listener) { + return typeof listener === 'function' ? listener : listener.onDidDispatch } From b7c328fd5961d039b73c9ceacc8f065e60ac8841 Mon Sep 17 00:00:00 2001 From: Will Binns-Smith Date: Mon, 28 Aug 2017 14:47:12 -0700 Subject: [PATCH 6/6] Clean up docs and comment wording --- spec/command-registry-spec.js | 18 +++++++++--------- src/command-registry.js | 26 ++++++++++++++------------ 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/spec/command-registry-spec.js b/spec/command-registry-spec.js index 5dbd8d14d..a0ac86c08 100644 --- a/spec/command-registry-spec.js +++ b/spec/command-registry-spec.js @@ -232,7 +232,7 @@ describe("CommandRegistry", () => { }).toThrow(new Error('Cannot register a command with a null listener.')); }); - it("throws an error when called with an null callback and object target", () => { + it("throws an error when called with a null callback and object target", () => { const badCallback = null; expect(() => { @@ -240,20 +240,20 @@ describe("CommandRegistry", () => { }).toThrow(new Error('Cannot register a command with a null listener.')); }); - it("throws an error when called with an object listener without a onDidDispatch method", () => { + it("throws an error when called with an object listener without a didDispatch method", () => { const badListener = { - title: 'a listener without a onDidDispatch callback', + title: 'a listener without a didDispatch callback', description: 'this should throw an error' }; expect(() => { registry.add(document.body, 'foo:bar', badListener); - }).toThrow(new Error('Listener must be a callback function or an object with a onDidDispatch method.')); + }).toThrow(new Error('Listener must be a callback function or an object with a didDispatch method.')); }); }); describe("::findCommands({target})", () => { - it("returns commands that can be invoked on the target or its ancestors", () => { + it("returns command descriptors that can be invoked on the target or its ancestors", () => { registry.add('.parent', 'namespace:command-1', () => {}); registry.add('.child', 'namespace:command-2', () => {}); registry.add('.grandchild', 'namespace:command-3', () => {}); @@ -273,7 +273,7 @@ describe("CommandRegistry", () => { ]); }); - it("returns commands with arbitrary metadata if set in a listener object", () => { + it("returns command descriptors with arbitrary metadata if set in a listener object", () => { registry.add('.grandchild', 'namespace:command-1', () => {}); registry.add('.grandchild', 'namespace:command-2', { displayName: 'Custom Command 2', @@ -281,7 +281,7 @@ describe("CommandRegistry", () => { some: 'other', object: 'data' }, - onDidDispatch() {} + didDispatch() {} }); registry.add('.grandchild', 'namespace:command-3', { name: 'some:other:incorrect:commandname', @@ -290,7 +290,7 @@ describe("CommandRegistry", () => { some: 'other', object: 'data' }, - onDidDispatch() {} + didDispatch() {} }); const commands = registry.findCommands({target: grandchild}); @@ -318,7 +318,7 @@ describe("CommandRegistry", () => { ]); }); - it("returns commands with arbitrary metadata if set on a listener function", () => { + it("returns command descriptors with arbitrary metadata if set on a listener function", () => { function listener () {} listener.displayName = 'Custom Command 2' listener.metadata = { diff --git a/src/command-registry.js b/src/command-registry.js index 5e4553603..30089b7f1 100644 --- a/src/command-registry.js +++ b/src/command-registry.js @@ -91,10 +91,12 @@ module.exports = class CommandRegistry { // handle such as `user:insert-date`. // * `listener` A listener which handles the event. Either A {Function} to // call when the given command is invoked on an element matching the - // selector, or an {Object} with a `onDidDispatch` property which is such a + // selector, or an {Object} with a `didDispatch` property which is such a // function. // - // It will be called with `this` referencing the matching DOM node. + // The function (`listener` itself if it is a function, or the `didDispatch` + // method if `listener` is an object) will be called with `this` referencing + // the matching DOM node and the following argument: // * `event` A standard DOM event instance. Call `stopPropagation` or // `stopImmediatePropagation` to terminate bubbling early. // @@ -137,10 +139,10 @@ module.exports = class CommandRegistry { // type Listener = ((e: CustomEvent) => void) | { // displayName?: string, // description?: string, - // onDidDispatch(e: CustomEvent): void, + // didDispatch(e: CustomEvent): void, // } - if ((typeof listener !== 'function') && (typeof listener.onDidDispatch !== 'function')) { - throw new Error('Listener must be a callback function or an object with a onDidDispatch method.') + if ((typeof listener !== 'function') && (typeof listener.didDispatch !== 'function')) { + throw new Error('Listener must be a callback function or an object with a didDispatch method.') } if (typeof target === 'string') { @@ -376,7 +378,7 @@ module.exports = class CommandRegistry { if (immediatePropagationStopped) { break } - listener.onDidDispatch.call(currentTarget, dispatchedEvent) + listener.didDispatch.call(currentTarget, dispatchedEvent) } if (currentTarget === window) { @@ -403,12 +405,12 @@ module.exports = class CommandRegistry { // type Listener = { // descriptor: CommandDescriptor, -// extractOnDidDispatch: (e: CustomEvent) => void, +// extractDidDispatch: (e: CustomEvent) => void, // }; class SelectorBasedListener { constructor (selector, commandName, listener) { this.selector = selector - this.onDidDispatch = extractOnDidDispatch(listener) + this.didDispatch = extractDidDispatch(listener) this.descriptor = extractDescriptor(commandName, listener) this.specificity = calculateSpecificity(this.selector) this.sequenceNumber = SequenceCount++ @@ -428,7 +430,7 @@ class SelectorBasedListener { class InlineListener { constructor (commandName, listener) { - this.onDidDispatch = extractOnDidDispatch(listener) + this.didDispatch = extractDidDispatch(listener) this.descriptor = extractDescriptor(commandName, listener) } } @@ -439,7 +441,7 @@ class InlineListener { // }; function extractDescriptor (name, listener) { return Object.assign( - _.omit(listener, 'onDidDispatch'), + _.omit(listener, 'didDispatch'), { name, displayName: listener.displayName ? listener.displayName : _.humanizeEventName(name) @@ -447,6 +449,6 @@ function extractDescriptor (name, listener) { ) } -function extractOnDidDispatch (listener) { - return typeof listener === 'function' ? listener : listener.onDidDispatch +function extractDidDispatch (listener) { + return typeof listener === 'function' ? listener : listener.didDispatch }