API alterations to allow injection of hotkey parsing and event processing
keithamus opened this issue · 4 comments
I think a useful migration strategy would be to allow serialization/comparison to be overridden as part of the API.
When we talk about what this library does that is "novel" (as in, something you cannot reasonably achieve in a few LOC locally) the library does three things:
- It manages many shortcuts by implementing a Radix Trie.
- It manages event listening & dispatch avoiding footguns: for example delegated events, ensuring hotkeys aren't fired on form fields, etc.
- It provides the above with a simple
install
/uninstall
API to simplify adding/removing event listeners and state from the Radix Trie. - It comes up with some lose specification of how to expand/compare hotkey strings to decide when to fire a hotkey combo.
The last one is what causes us a lot of trouble and causes some trashing in this library. radix-trie.ts
hasn't been touched in 9 months, prior to that 2 years ago. I'd say radix-trie
is "feature complete". Meanwhile hotkey.ts
has a regular cadence of alterations every few months as we reach edge cases and scale our use of this library.
The chief problems with the serialization format are:
- It is a psuedo specification. There's no formal set of possible values or a well defined grammar or spec. It is an ad-hoc grammar using RegExps. While this works fine for the most part, it is a source of bugs and confusion, as well as differences of opinion.
- It doesn't properly encode all of the state about what we as developers intend shortcuts to be. We've discussed this quite a lot synchronously; the concept of "logical" (
WSAD
) vs "Semiotic" (?
) shortcuts.
Effectively the serialization of these shortcuts is, what you might call, unsolved. So I say let's make that apparent by allowing it to be overridden in the API.
The current API is as follows:
export install(element: HTMLElement, hotkey?: string): void {}
export uninstall(element: HTMLElement): void {}
I propose we expand this to the following:
export type ProcessHotkey = (hotkey: string): string[][]
export type ProcessEvent = (event: KeyboardEvent): string
export class HotkeyManager {
constructor(
processHotkey: ProcessHotkey = expandHotkeyToEdges,
processEvent: ProcessEvent = eventToHotkeyString
)
install(element: HTMLelement, hotkey?: string): void {}
uninstall(element: HTMLElement): void {}
}
const defaultManager = new HotkeyManager()
export const install = defaultManager.install
export const uninstall = defaultManager.uninstall
By making a class, we can define custom processing for shortcut keys which allows users to define their own shortcut models, but also allows us to make more breaking changes behind experimental APIs, and even feature flag them. By still exposing the install
/uninstall
functions per the existing API, we ensure backwards compatibility which minimizes breaking changes, and allows us to "make the hard change easy then make the easy change". A 2.0 change could effectively swap the default hotkey functions out for the newer API, as a one line change.
Thoughts @github/ui-frameworks?
I think this is a great plan going forward and I've added it to the v2 tracking issue #65!
One quibble I'd make with this API though is around the naming of processHotkey
vs processExpansion
. There are indeed these two serializers, however I would change the names:
- During
installation
we read thedata-hotkey
value and store that internally (in a trie). I would call this functionprocessHotkey
. - When handling events, we need to serialize the event into a string to compare to the hotkey trie node: I would call this
processEvent
Notice that these names align with the function argument names you have already proposed.
I also wonder if we want to use a class approach: One concern I have with the class approach is that I would expect that state would be managed in the class instance, but we currently manage state in module-scoped private variables.
I think we could continue with a functional approach, but pass a config object to the install
command. I don't think the uninstall command depends on processHotkey
or processEvent
.
The API, then could be
export type ProcessHotkey = (hotkey: string): string[][]
export type ProcessEvent = (event: KeyboardEvent): string
export interface HotkeyOptions {
hotkey?: string;
processHotkey: ProcessHotkey;
processEvent: ProcessEvent;
}
export function install(
element: HTMLelement,
options = {
hotkey,
processHotkey: expandHotkeyToEdges,
processEvent: eventToHotkeyString;
}: HotkeyOptions
): void {}
export function uninstall(element: HTMLElement): void {}
This would almost not even be a breaking change — except I had to move the optional hotkey
argument to install
into the HotkeyOptions
.
Anyways, I am okay with either approach in the end: just thought I would throw this option out there.
One quibble I'd make with this API though is around the naming of
processHotkey
vsprocessExpansion
Your suggestion here LGTM 👍
I think we could continue with a functional approach, but pass a config object to the
install
command
The problem with that proposal is less to do with uninstall
and more to do with the internalizing of the processors in respect to stored state. Having a functional approach where users can change what processHotkey
and processExpansion
across multiple install calls leaves issues with how Leafs are processed in the radix trie, and how event delegation would work. Another way to say this is that each combination of processHotkey
+processExpansion
needs its own event manager/radix trie.
Having a functional approach where users can change what
processHotkey
andprocessExpansion
across multiple install calls leaves issues with how Leafs are processed in the radix trie, and how event delegation would work.
Yeah! I totally agree with this. I am onboard with the class.
(I updated the code in your description with the changed processor names)