From 4c039da15eeea62ce291c173e673c5aa07678cfc Mon Sep 17 00:00:00 2001 From: Sebastian Malton Date: Thu, 23 Jun 2022 14:46:04 -0400 Subject: [PATCH] Remove reaction and replace with ObservableCrate - This new abstraction allows us to hook into the transitions between values without having to resort to reactions. Allowing us to be explicit in when we want code to execute and also be defensive against new code paths Signed-off-by: Sebastian Malton --- src/common/utils/index.ts | 9 +- src/common/utils/noop.ts | 10 + src/common/utils/observable-crate/impl.ts | 53 ++++ .../observable-crate/observable-crate.test.ts | 86 ++++++ .../is-selection-key.injectable.ts | 22 ++ ...amespace-select-filter-model.injectable.ts | 8 +- .../namespace-select-filter-model.tsx | 272 +++++++++--------- .../+namespaces/namespace-select-filter.tsx | 5 +- 8 files changed, 310 insertions(+), 155 deletions(-) create mode 100644 src/common/utils/noop.ts create mode 100644 src/common/utils/observable-crate/impl.ts create mode 100644 src/common/utils/observable-crate/observable-crate.test.ts create mode 100644 src/renderer/components/+namespaces/namespace-select-filter-model/is-selection-key.injectable.ts diff --git a/src/common/utils/index.ts b/src/common/utils/index.ts index e36ccd317f..f0aa66294b 100644 --- a/src/common/utils/index.ts +++ b/src/common/utils/index.ts @@ -3,13 +3,6 @@ * Licensed under MIT License. See LICENSE in root directory for more information. */ -/** - * A function that does nothing - */ -export function noop(...args: T): void { - return void args; -} - export * from "./abort-controller"; export * from "./app-version"; export * from "./autobind"; @@ -27,6 +20,8 @@ export * from "./formatDuration"; export * from "./getRandId"; export * from "./hash-set"; export * from "./n-fircate"; +export * from "./noop"; +export * from "./observable-crate/impl"; export * from "./openBrowser"; export * from "./paths"; export * from "./promise-exec"; diff --git a/src/common/utils/noop.ts b/src/common/utils/noop.ts new file mode 100644 index 0000000000..a9171f2618 --- /dev/null +++ b/src/common/utils/noop.ts @@ -0,0 +1,10 @@ +/** + * Copyright (c) OpenLens Authors. All rights reserved. + * Licensed under MIT License. See LICENSE in root directory for more information. + */ +/** + * A function that does nothing + */ +export function noop(...args: T): void { + return void args; +} diff --git a/src/common/utils/observable-crate/impl.ts b/src/common/utils/observable-crate/impl.ts new file mode 100644 index 0000000000..e9874c9b7d --- /dev/null +++ b/src/common/utils/observable-crate/impl.ts @@ -0,0 +1,53 @@ +/** + * Copyright (c) OpenLens Authors. All rights reserved. + * Licensed under MIT License. See LICENSE in root directory for more information. + */ + +import { observable, runInAction } from "mobx"; +import { getOrInsertMap } from "../collection-functions"; +import { noop } from "../noop"; + +export interface ObservableCrate { + get(): T; + set(value: T): void; +} + +export interface ObservableCrateFactory { + (initialValue: T, transitionHandlers?: ObservableCrateTransitionHandlers): ObservableCrate; +} + +export interface ObservableCrateTransitionHandler { + from: T; + to: T; + onTransition: () => void; +} +export type ObservableCrateTransitionHandlers = ObservableCrateTransitionHandler[]; + +function convertToHandlersMap(handlers: ObservableCrateTransitionHandlers): Map void>> { + const res: ReturnType> = new Map(); + + for (const { from, to, onTransition } of handlers) { + getOrInsertMap(res, from).set(to, onTransition); + } + + return res; +} + +export const observableCrate = ((initialValue, transitionHandlers = []) => { + const crate = observable.box(initialValue); + const handlers = convertToHandlersMap(transitionHandlers); + + return { + get() { + return crate.get(); + }, + set(value) { + const onTransition = handlers.get(crate.get())?.get(value) ?? noop; + + runInAction(() => { + crate.set(value); + onTransition(); + }); + }, + }; +}) as ObservableCrateFactory; diff --git a/src/common/utils/observable-crate/observable-crate.test.ts b/src/common/utils/observable-crate/observable-crate.test.ts new file mode 100644 index 0000000000..9e6c29a35e --- /dev/null +++ b/src/common/utils/observable-crate/observable-crate.test.ts @@ -0,0 +1,86 @@ +/** + * Copyright (c) OpenLens Authors. All rights reserved. + * Licensed under MIT License. See LICENSE in root directory for more information. + */ + +import type { ObservableCrate } from "./impl"; +import { observableCrate } from "./impl"; + +describe("observable-crate", () => { + it("can be constructed with initial value", () => { + expect(() => observableCrate(0).build()).not.toThrow(); + }); + + it("has a definite type if the initial value is provided", () => { + expect (() => { + const res: ObservableCrate = observableCrate(0).build(); + + void res; + }).not.toThrow(); + }); + + it("accepts a map of transitionHandlers", () => { + expect(() => observableCrate(0).withHandlers(new Map())).not.toThrow(); + }); + + describe("with a crate over an enum, and some transition handlers", () => { + enum Test { + Start, + T1, + End, + } + + let crate: ObservableCrate; + let correctHandler: jest.MockedFunction<() => void>; + let incorrectHandler: jest.MockedFunction<() => void>; + + beforeEach(() => { + correctHandler = jest.fn(); + incorrectHandler = jest.fn(); + crate = observableCrate(Test.Start).withHandlers(new Map([ + [Test.Start, new Map([ + [Test.Start, incorrectHandler], + [Test.T1, correctHandler], + [Test.End, incorrectHandler], + ])], + [Test.T1, new Map([ + [Test.Start, incorrectHandler], + [Test.T1, incorrectHandler], + [Test.End, incorrectHandler], + ])], + [Test.End, new Map([ + [Test.Start, incorrectHandler], + [Test.T1, incorrectHandler], + [Test.End, incorrectHandler], + ])], + ])); + }); + + it("initial value is available", () => { + expect(crate.get()).toBe(Test.Start); + }); + + it("does not call any transition handler", () => { + expect(correctHandler).not.toBeCalled(); + expect(incorrectHandler).not.toBeCalled(); + }); + + describe("when setting a new value", () => { + beforeEach(() => { + crate.set(Test.T1); + }); + + it("calls the associated transition handler", () => { + expect(correctHandler).toBeCalled(); + }); + + it("does not call any other transition handler", () => { + expect(incorrectHandler).not.toBeCalled(); + }); + + it("new value is available", () => { + expect(crate.get()).toBe(Test.T1); + }); + }); + }); +}); diff --git a/src/renderer/components/+namespaces/namespace-select-filter-model/is-selection-key.injectable.ts b/src/renderer/components/+namespaces/namespace-select-filter-model/is-selection-key.injectable.ts new file mode 100644 index 0000000000..3562b8a166 --- /dev/null +++ b/src/renderer/components/+namespaces/namespace-select-filter-model/is-selection-key.injectable.ts @@ -0,0 +1,22 @@ +/** + * Copyright (c) OpenLens Authors. All rights reserved. + * Licensed under MIT License. See LICENSE in root directory for more information. + */ +import { getInjectable } from "@ogre-tools/injectable"; +import type React from "react"; +import isMacInjectable from "../../../../common/vars/is-mac.injectable"; + +export type IsMultiSelectionKey = (event: React.KeyboardEvent) => boolean; + +const isMultiSelectionKeyInjectable = getInjectable({ + id: "is-multi-selection-key", + instantiate: (di): IsMultiSelectionKey => { + const isMac = di.inject(isMacInjectable); + + return isMac + ? ({ key }) => key === "Meta" + : ({ key }) => key === "Control"; + }, +}); + +export default isMultiSelectionKeyInjectable; diff --git a/src/renderer/components/+namespaces/namespace-select-filter-model/namespace-select-filter-model.injectable.ts b/src/renderer/components/+namespaces/namespace-select-filter-model/namespace-select-filter-model.injectable.ts index 5b1ed84174..5578767cf1 100644 --- a/src/renderer/components/+namespaces/namespace-select-filter-model/namespace-select-filter-model.injectable.ts +++ b/src/renderer/components/+namespaces/namespace-select-filter-model/namespace-select-filter-model.injectable.ts @@ -2,17 +2,17 @@ * Copyright (c) OpenLens Authors. All rights reserved. * Licensed under MIT License. See LICENSE in root directory for more information. */ -import { NamespaceSelectFilterModel } from "./namespace-select-filter-model"; +import { namespaceSelectFilterModelFor } from "./namespace-select-filter-model"; import { getInjectable } from "@ogre-tools/injectable"; import namespaceStoreInjectable from "../store.injectable"; -import isMacInjectable from "../../../../common/vars/is-mac.injectable"; +import isMultiSelectionKeyInjectable from "./is-selection-key.injectable"; const namespaceSelectFilterModelInjectable = getInjectable({ id: "namespace-select-filter-model", - instantiate: (di) => new NamespaceSelectFilterModel({ + instantiate: (di) => namespaceSelectFilterModelFor({ namespaceStore: di.inject(namespaceStoreInjectable), - isMac: di.inject(isMacInjectable), + isMultiSelectionKey: di.inject(isMultiSelectionKeyInjectable), }), }); diff --git a/src/renderer/components/+namespaces/namespace-select-filter-model/namespace-select-filter-model.tsx b/src/renderer/components/+namespaces/namespace-select-filter-model/namespace-select-filter-model.tsx index 718790cb88..5bdfa3b479 100644 --- a/src/renderer/components/+namespaces/namespace-select-filter-model/namespace-select-filter-model.tsx +++ b/src/renderer/components/+namespaces/namespace-select-filter-model/namespace-select-filter-model.tsx @@ -3,16 +3,18 @@ * Licensed under MIT License. See LICENSE in root directory for more information. */ import React from "react"; -import { observable, action, computed, makeObservable, comparer, reaction } from "mobx"; +import type { IComputedValue } from "mobx"; +import { observable, action, computed, comparer } from "mobx"; import type { NamespaceStore } from "../store"; -import type { ActionMeta } from "react-select"; +import type { ActionMeta, MultiValue } from "react-select"; import { Icon } from "../../icon"; import type { SelectOption } from "../../select"; -import { autoBind } from "../../../utils"; +import { observableCrate } from "../../../utils"; +import type { IsMultiSelectionKey } from "./is-selection-key.injectable"; interface Dependencies { - readonly namespaceStore: NamespaceStore; - readonly isMac: boolean; + namespaceStore: NamespaceStore; + isMultiSelectionKey: IsMultiSelectionKey; } export const selectAllNamespaces = Symbol("all-namespaces-selected"); @@ -20,50 +22,71 @@ export const selectAllNamespaces = Symbol("all-namespaces-selected"); export type SelectAllNamespaces = typeof selectAllNamespaces; export type NamespaceSelectFilterOption = SelectOption; -export class NamespaceSelectFilterModel { - private isSelectionKey = (event: React.KeyboardEvent): boolean => { - if (this.dependencies.isMac) { - return event.key === "Meta"; - } - - return event.key === "Control"; // windows or linux - }; - - constructor(private readonly dependencies: Dependencies) { - makeObservable(this); - autoBind(this); - - reaction( - () => this.menuIsOpen.get(), - (isOpen) => { - if (!isOpen) { // falling edge of menu being open - this.optionsSortingSelected.replace(this.selectedNames.get()); - this.didToggle = false; - } - }, - ); +export function formatOptionLabel({ value, isSelected }: NamespaceSelectFilterOption) { + if (value === selectAllNamespaces) { + return <>All Namespaces; } - readonly menuIsOpen = observable.box(false); + return ( +
+ + {value} + {isSelected && ( + + )} +
+ ); +} - private readonly selectedNames = computed(() => new Set(this.dependencies.namespaceStore.contextNamespaces), { +export interface NamespaceSelectFilterModel { + readonly options: IComputedValue; + readonly menu: { + open: () => void; + close: () => void; + readonly isOpen: boolean; + }; + onChange: (newValue: MultiValue, actionMeta: ActionMeta) => void; + onClick: () => void; + onKeyDown: React.KeyboardEventHandler; + onKeyUp: React.KeyboardEventHandler; + reset: () => void; +} + +enum SelectMenuState { + Close = "close", + Open = "open", +} + +export function namespaceSelectFilterModelFor(dependencies: Dependencies): NamespaceSelectFilterModel { + const { isMultiSelectionKey, namespaceStore } = dependencies; + + let didToggle = false; + let isMultiSelection = false; + const menuState = observableCrate(SelectMenuState.Close, [{ + from: SelectMenuState.Close, + to: SelectMenuState.Open, + onTransition: () => { + optionsSortingSelected.replace(selectedNames.get()); + didToggle = false; + }, + }]); + const selectedNames = computed(() => new Set(namespaceStore.contextNamespaces), { equals: comparer.structural, }); - - /** - * This set is only updated on the falling edge of the menu being open. That way while the menu is - * open the order of the items doesn't change - */ - private readonly optionsSortingSelected = observable.set(this.selectedNames.get()); - - readonly options = computed((): readonly NamespaceSelectFilterOption[] => { - const baseOptions = this.dependencies.namespaceStore.items.map(ns => ns.getName()); - const selectedNames = this.selectedNames.get(); + const optionsSortingSelected = observable.set(selectedNames.get()); + const options = computed((): readonly NamespaceSelectFilterOption[] => { + const baseOptions = namespaceStore.items.map(ns => ns.getName()); + const namespaces = selectedNames.get(); baseOptions.sort(( (left, right) => - +this.optionsSortingSelected.has(right) - - +this.optionsSortingSelected.has(left) + +optionsSortingSelected.has(right) + - +optionsSortingSelected.has(left) )); return [ @@ -77,113 +100,78 @@ export class NamespaceSelectFilterModel { value: namespace, label: namespace, id: namespace, - isSelected: selectedNames.has(namespace), + isSelected: namespaces.has(namespace), })), ]; }); + const menuIsOpen = computed(() => menuState.get() === SelectMenuState.Open); - formatOptionLabel({ value, isSelected }: NamespaceSelectFilterOption) { - if (value === selectAllNamespaces) { - return "All Namespaces"; - } - - return ( -
- - {value} - {isSelected && ( - - )} -
- ); - } - - @action - closeMenu() { - this.menuIsOpen.set(false); - } - - @action - openMenu(){ - this.menuIsOpen.set(true); - } - - isSelected(namespace: string | string[]) { - return this.dependencies.namespaceStore.hasContext(namespace); - } - - selectSingle(namespace: string) { - this.dependencies.namespaceStore.selectSingle(namespace); - } - - selectAll() { - this.dependencies.namespaceStore.selectAll(); - } - - onChange(namespace: unknown, action: ActionMeta) { - switch (action.action) { - case "clear": - this.dependencies.namespaceStore.selectAll(); - break; - case "deselect-option": - if (typeof action.option === "string") { - this.didToggle = true; - this.dependencies.namespaceStore.toggleSingle(action.option); - } - break; - case "select-option": - if (action.option?.value === selectAllNamespaces) { - this.didToggle = true; - this.dependencies.namespaceStore.selectAll(); - } else if (action.option) { - this.didToggle = true; - - if (this.isMultiSelection) { - this.dependencies.namespaceStore.toggleSingle(action.option.value); - } else { - this.dependencies.namespaceStore.selectSingle(action.option.value); + const model: NamespaceSelectFilterModel = { + options, + menu: { + close: action(() => { + menuState.set(SelectMenuState.Close); + }), + open: action(() => { + menuState.set(SelectMenuState.Open); + }), + get isOpen() { + return menuIsOpen.get(); + }, + }, + onChange: (_, action) => { + switch (action.action) { + case "clear": + namespaceStore.selectAll(); + break; + case "deselect-option": + if (typeof action.option === "string") { + didToggle = true; + namespaceStore.toggleSingle(action.option); } - } - break; - } - } + break; + case "select-option": + if (action.option?.value === selectAllNamespaces) { + didToggle = true; + namespaceStore.selectAll(); + } else if (action.option) { + didToggle = true; - onClick() { - if (!this.menuIsOpen.get()) { - this.openMenu(); - } else if (!this.isMultiSelection) { - this.closeMenu(); - } - } - - private isMultiSelection = false; - - onKeyDown(event: React.KeyboardEvent) { - if (this.isSelectionKey(event)) { - this.isMultiSelection = true; - } - } - - private didToggle = false; - - onKeyUp(event: React.KeyboardEvent) { - if (this.isSelectionKey(event)) { - this.isMultiSelection = false; - - if (this.didToggle) { - this.closeMenu(); + if (isMultiSelection) { + namespaceStore.toggleSingle(action.option.value); + } else { + namespaceStore.selectSingle(action.option.value); + } + } + break; } - } - } + }, + onClick: () => { + if (!menuIsOpen.get()) { + model.menu.open(); + } else if (!isMultiSelection) { + model.menu.close(); + } + }, + onKeyDown: (event) => { + if (isMultiSelectionKey(event)) { + isMultiSelection = true; + } + }, + onKeyUp: (event) => { + if (isMultiSelectionKey(event)) { + isMultiSelection = false; - @action - reset() { - this.isMultiSelection = false; - this.closeMenu(); - } + if (didToggle) { + model.menu.close(); + } + } + }, + reset: action(() => { + isMultiSelection = false; + model.menu.close(); + }), + }; + + return model; } diff --git a/src/renderer/components/+namespaces/namespace-select-filter.tsx b/src/renderer/components/+namespaces/namespace-select-filter.tsx index 969cf9f917..a5deafc60b 100644 --- a/src/renderer/components/+namespaces/namespace-select-filter.tsx +++ b/src/renderer/components/+namespaces/namespace-select-filter.tsx @@ -13,6 +13,7 @@ import type { NamespaceStore } from "./store"; import { Select } from "../select"; import { withInjectables } from "@ogre-tools/injectable-react"; import type { NamespaceSelectFilterModel, NamespaceSelectFilterOption, SelectAllNamespaces } from "./namespace-select-filter-model/namespace-select-filter-model"; +import { formatOptionLabel } from "./namespace-select-filter-model/namespace-select-filter-model"; import namespaceSelectFilterModelInjectable from "./namespace-select-filter-model/namespace-select-filter-model.injectable"; import namespaceStoreInjectable from "./store.injectable"; @@ -36,13 +37,13 @@ const NonInjectedNamespaceSelectFilter = observer(({ model, id }: Dependencies & id={id} isMulti={true} isClearable={false} - menuIsOpen={model.menuIsOpen.get()} + menuIsOpen={model.menu.isOpen} components={{ Placeholder }} closeMenuOnSelect={false} controlShouldRenderValue={false} onChange={model.onChange} onBlur={model.reset} - formatOptionLabel={model.formatOptionLabel} + formatOptionLabel={formatOptionLabel} options={model.options.get()} className="NamespaceSelect NamespaceSelectFilter" menuClass="NamespaceSelectFilterMenu"