rewrite HotkeysTarget and ContextMenuTarget as HOCs

packages/core/src/common/constructor.ts

@@ -0,0 +1,13 @@
+/*
+ * Copyright 2017 Palantir Technologies, Inc. All rights reserved.
+ *
+ * Licensed under the terms of the LICENSE file distributed with this project.
+ */
+
+/**
+ * Generic interface defining constructor types, such as classes. This is used to type the class
+ * itself in meta-programming situations such as decorators.
+ */
+export interface IConstructor<T> {
+    new (...args: any[]): T;
+}

packages/core/src/common/errors.ts

@@ -15,8 +15,13 @@ export const COLLAPSIBLE_LIST_INVALID_CHILD = ns + ` <CollapsibleList> children
 
 export const CONTEXTMENU_WARN_DECORATOR_NO_METHOD =
     ns + ` @ContextMenuTarget-decorated class should implement renderContextMenu.`;
+export const CONTEXTMENU_WARN_DECORATOR_NEEDS_REACT_ELEMENT =
+    ns + ` "@ContextMenuTarget-decorated components must return a single JSX.Element or an empty render.`;
 
 export const HOTKEYS_HOTKEY_CHILDREN = ns + ` <Hotkeys> only accepts <Hotkey> children.`;
+export const HOTKEYS_WARN_DECORATOR_NO_METHOD = ns + ` @HotkeysTarget-decorated class should implement renderHotkeys.`;
+export const HOTKEYS_WARN_DECORATOR_NEEDS_REACT_ELEMENT =
+    ns + ` "@HotkeysTarget-decorated components must return a single JSX.Element or an empty render.`;
 
 export const MENU_WARN_CHILDREN_SUBMENU_MUTEX =
     ns + ` <MenuItem> children and submenu props are mutually exclusive, with children taking priority.`;

packages/core/src/common/index.ts

@@ -6,6 +6,7 @@
 
 export * from "./abstractComponent";
 export * from "./colors";
+export * from "./constructor";
 export * from "./intent";
 export * from "./position";
 export * from "./props";

packages/core/src/common/utils.ts

@@ -22,6 +22,17 @@ export function isFunction(value: any): value is Function {
     return typeof value === "function";
 }
 
+/**
+ * Represents anything that has a `name` property such as Functions.
+ */
+export interface INamed {
+    name?: string;
+}
+
+export function getDisplayName(ComponentClass: React.ComponentClass | INamed) {
+    return (ComponentClass as React.ComponentClass).displayName || (ComponentClass as INamed).name || "Unknown";
+}
+
 /**
  * Safely invoke the function with the given arguments, if it is indeed a
  * function, and return its value.

packages/core/src/components/context-menu/contextMenuTarget.tsx

@@ -7,55 +7,70 @@
 import * as React from "react";
 import * as ReactDOM from "react-dom";
 
-import { CONTEXTMENU_WARN_DECORATOR_NO_METHOD } from "../../common/errors";
-import { isFunction, safeInvoke } from "../../common/utils";
+import { IConstructor } from "../../common/constructor";
+import {
+    CONTEXTMENU_WARN_DECORATOR_NEEDS_REACT_ELEMENT,
+    CONTEXTMENU_WARN_DECORATOR_NO_METHOD,
+} from "../../common/errors";
+import { getDisplayName, isFunction, safeInvoke } from "../../common/utils";
 import { isDarkTheme } from "../../common/utils/isDarkTheme";
 import * as ContextMenu from "./contextMenu";
 
 export interface IContextMenuTarget extends React.Component<any, any> {
+    render(): React.ReactElement<any> | null | undefined;
     renderContextMenu(e: React.MouseEvent<HTMLElement>): JSX.Element | undefined;
     onContextMenuClose?(): void;
 }
 
-export function ContextMenuTarget<T extends { prototype: IContextMenuTarget }>(constructor: T) {
-    const { render, renderContextMenu, onContextMenuClose } = constructor.prototype;
-
-    if (!isFunction(renderContextMenu)) {
+export function ContextMenuTarget<T extends IConstructor<IContextMenuTarget>>(WrappedComponent: T) {
+    if (!isFunction(WrappedComponent.prototype.renderContextMenu)) {
         console.warn(CONTEXTMENU_WARN_DECORATOR_NO_METHOD);
     }
 
-    // patching classes like this requires preserving function context
-    // tslint:disable-next-line only-arrow-functions
-    constructor.prototype.render = function(this: IContextMenuTarget) {
-        /* tslint:disable:no-invalid-this */
-        const element = render.call(this) as JSX.Element;
+    return class ContextMenuTargetClass extends WrappedComponent {
+        public static displayName = `ContextMenuTarget(${getDisplayName(WrappedComponent)})`;
 
-        if (element == null) {
-            // always return `element` in case caller is distinguishing between `null` and `undefined`
-            return element;
-        }
+        public render() {
+            const element = super.render();
 
-        const oldOnContextMenu = element.props.onContextMenu as React.MouseEventHandler<HTMLElement>;
-        const onContextMenu = (e: React.MouseEvent<HTMLElement>) => {
-            // support nested menus (inner menu target would have called preventDefault())
-            if (e.defaultPrevented) {
-                return;
+            if (element == null) {
+                // always return `element` in case caller is distinguishing between `null` and `undefined`
+                return element;
             }
 
-            if (isFunction(this.renderContextMenu)) {
-                const menu = this.renderContextMenu(e);
-                if (menu != null) {
-                    const htmlElement = ReactDOM.findDOMNode(this);
-                    const darkTheme = htmlElement != null && isDarkTheme(htmlElement);
-                    e.preventDefault();
-                    ContextMenu.show(menu, { left: e.clientX, top: e.clientY }, onContextMenuClose, darkTheme);
-                }
+            if (!React.isValidElement<any>(element)) {
+                console.warn(CONTEXTMENU_WARN_DECORATOR_NEEDS_REACT_ELEMENT);
+                return element;
             }
+            const oldOnContextMenu = element.props.onContextMenu as React.MouseEventHandler<HTMLElement>;
+            const onContextMenu = (e: React.MouseEvent<HTMLElement>) => {
+                // support nested menus (inner menu target would have called preventDefault())
+                if (e.defaultPrevented) {
+                    return;
+                }
 
-            safeInvoke(oldOnContextMenu, e);
-        };
+                if (isFunction(this.renderContextMenu)) {
+                    const menu = this.renderContextMenu(e);
+                    if (menu != null) {
+                        const htmlElement = ReactDOM.findDOMNode(this);
+                        const darkTheme = htmlElement != null && isDarkTheme(htmlElement);
+                        e.preventDefault();
+                        ContextMenu.show(
+                            menu,
+                            {
+                                left: e.clientX,
+                                top: e.clientY,
+                            },
+                            this.onContextMenuClose,
+                            darkTheme,
+                        );
+                    }
+                }
+
+                safeInvoke(oldOnContextMenu, e);
+            };
 
-        return React.cloneElement(element, { onContextMenu });
-        /* tslint:enable:no-invalid-this */
+            return React.cloneElement(element, { onContextMenu });
+        }
     };
 }

packages/core/src/components/hotkeys/hotkeysEvents.ts

@@ -37,7 +37,7 @@ export class HotkeysEvents {
         this.actions = [];
     }
 
-    public setHotkeys(props: IHotkeysProps & { children: ReactNode[] }) {
+    public setHotkeys(props: IHotkeysProps & { children?: ReactNode }) {
         const actions = [] as IHotkeyAction[];
         Children.forEach(props.children, (child: ReactElement<any>) => {
             if (Hotkey.isInstance(child) && this.isScope(child.props)) {

packages/core/src/components/hotkeys/hotkeysTarget.tsx

@@ -5,97 +5,100 @@
  */
 
 import * as React from "react";
-import { isFunction, safeInvoke } from "../../common/utils";
 
+import { IConstructor } from "../../common/constructor";
+import { HOTKEYS_WARN_DECORATOR_NEEDS_REACT_ELEMENT, HOTKEYS_WARN_DECORATOR_NO_METHOD } from "../../common/errors";
+import { getDisplayName, isFunction, safeInvoke } from "../../common/utils";
 import { IHotkeysProps } from "./hotkeys";
 import { HotkeyScope, HotkeysEvents } from "./hotkeysEvents";
 
 export interface IHotkeysTarget extends React.Component<any, any>, React.ComponentLifecycle<any, any> {
-    /** @internal */
-    globalHotkeysEvents?: HotkeysEvents;
-
-    /** @internal */
-    localHotkeysEvents?: HotkeysEvents;
-
+    render(): React.ReactElement<any> | null | undefined;
     /**
      * Components decorated with the `HotkeysTarget` decorator must implement
      * this method, and it must return a `Hotkeys` React element.
      */
     renderHotkeys(): React.ReactElement<IHotkeysProps>;
 }
 
-export function HotkeysTarget<T extends { prototype: IHotkeysTarget }>(constructor: T) {
-    const {
-        componentWillMount,
-        componentDidMount,
-        componentWillUnmount,
-        render,
-        renderHotkeys,
-    } = constructor.prototype;
-
-    if (!isFunction(renderHotkeys)) {
-        throw new Error(`@HotkeysTarget-decorated class must implement \`renderHotkeys\`. ${constructor}`);
+export function HotkeysTarget<T extends IConstructor<IHotkeysTarget>>(WrappedComponent: T) {
+    if (!isFunction(WrappedComponent.prototype.renderHotkeys)) {
+        console.warn(HOTKEYS_WARN_DECORATOR_NO_METHOD);
     }
 
-    // tslint:disable no-invalid-this only-arrow-functions
-    constructor.prototype.componentWillMount = function() {
-        this.localHotkeysEvents = new HotkeysEvents(HotkeyScope.LOCAL);
-        this.globalHotkeysEvents = new HotkeysEvents(HotkeyScope.GLOBAL);
+    return class HotkeysTargetClass extends WrappedComponent {
+        public static displayName = `HotkeysTarget(${getDisplayName(WrappedComponent)})`;
 
-        if (componentWillMount != null) {
-            componentWillMount.call(this);
-        }
-    };
+        /** @internal */
+        public globalHotkeysEvents?: HotkeysEvents;
 
-    constructor.prototype.componentDidMount = function() {
-        // attach global key event listeners
-        document.addEventListener("keydown", this.globalHotkeysEvents.handleKeyDown);
-        document.addEventListener("keyup", this.globalHotkeysEvents.handleKeyUp);
+        /** @internal */
+        public localHotkeysEvents?: HotkeysEvents;
 
-        if (componentDidMount != null) {
-            componentDidMount.call(this);
+        public componentWillMount() {
+            if (super.componentWillMount != null) {
+                super.componentWillMount();
+            }
+            this.localHotkeysEvents = new HotkeysEvents(HotkeyScope.LOCAL);
+            this.globalHotkeysEvents = new HotkeysEvents(HotkeyScope.GLOBAL);
         }
-    };
 
-    constructor.prototype.componentWillUnmount = function() {
-        // detach global key event listeners
-        document.removeEventListener("keydown", this.globalHotkeysEvents.handleKeyDown);
-        document.removeEventListener("keyup", this.globalHotkeysEvents.handleKeyUp);
+        public componentDidMount() {
+            if (super.componentDidMount != null) {
+                super.componentDidMount();
+            }
+
+            // attach global key event listeners
+            document.addEventListener("keydown", this.globalHotkeysEvents.handleKeyDown);
+            document.addEventListener("keyup", this.globalHotkeysEvents.handleKeyUp);
+        }
 
-        this.globalHotkeysEvents.clear();
-        this.localHotkeysEvents.clear();
+        public componentWillUnmount() {
+            if (super.componentWillUnmount != null) {
+                super.componentWillUnmount();
+            }
+            document.removeEventListener("keydown", this.globalHotkeysEvents.handleKeyDown);
+            document.removeEventListener("keyup", this.globalHotkeysEvents.handleKeyUp);
 
-        if (componentWillUnmount != null) {
-            componentWillUnmount.call(this);
+            this.globalHotkeysEvents.clear();
+            this.localHotkeysEvents.clear();
         }
-    };
 
-    constructor.prototype.render = function() {
-        const element = render.call(this) as JSX.Element;
-
-        const hotkeys = renderHotkeys.call(this);
-        this.localHotkeysEvents.setHotkeys(hotkeys.props);
-        this.globalHotkeysEvents.setHotkeys(hotkeys.props);
-
-        // attach local key event listeners
-        if (element != null && this.localHotkeysEvents.count() > 0) {
-            const tabIndex = hotkeys.props.tabIndex === undefined ? 0 : hotkeys.props.tabIndex;
-
-            const existingKeyDown = element.props.onKeyDown as React.KeyboardEventHandler<HTMLElement>;
-            const onKeyDown = (e: React.KeyboardEvent<HTMLElement>) => {
-                this.localHotkeysEvents.handleKeyDown(e.nativeEvent as KeyboardEvent);
-                safeInvoke(existingKeyDown, e);
-            };
-
-            const existingKeyUp = element.props.onKeyUp as React.KeyboardEventHandler<HTMLElement>;
-            const onKeyUp = (e: React.KeyboardEvent<HTMLElement>) => {
-                this.localHotkeysEvents.handleKeyUp(e.nativeEvent as KeyboardEvent);
-                safeInvoke(existingKeyUp, e);
-            };
-            return React.cloneElement(element, { tabIndex, onKeyDown, onKeyUp });
-        } else {
+        public render() {
+            const element = super.render() as JSX.Element;
+
+            if (element == null) {
+                // always return `element` in case caller is distinguishing between `null` and `undefined`
+                return element;
+            }
+
+            if (!React.isValidElement<any>(element)) {
+                console.warn(HOTKEYS_WARN_DECORATOR_NEEDS_REACT_ELEMENT);
+                return element;
+            }
+
+            if (isFunction(this.renderHotkeys)) {
+                const hotkeys = this.renderHotkeys();
+                this.localHotkeysEvents.setHotkeys(hotkeys.props);
+                this.globalHotkeysEvents.setHotkeys(hotkeys.props);
+
+                if (this.localHotkeysEvents.count() > 0) {
+                    const tabIndex = hotkeys.props.tabIndex === undefined ? 0 : hotkeys.props.tabIndex;
+
+                    const { keyDown: existingKeyDown, keyUp: existingKeyUp } = element.props;
+                    const onKeyDown = (e: React.KeyboardEvent<HTMLElement>) => {
+                        this.localHotkeysEvents.handleKeyDown(e.nativeEvent as KeyboardEvent);
+                        safeInvoke(existingKeyDown, e);
+                    };
+
+                    const onKeyUp = (e: React.KeyboardEvent<HTMLElement>) => {
+                        this.localHotkeysEvents.handleKeyUp(e.nativeEvent as KeyboardEvent);
+                        safeInvoke(existingKeyUp, e);
+                    };
+                    return React.cloneElement(element, { tabIndex, onKeyDown, onKeyUp });
+                }
+            }
             return element;
         }
     };
-    // tslint:enable
 }

packages/core/test/context-menu/contextMenuTests.tsx

@@ -4,6 +4,8 @@
  * Licensed under the terms of the LICENSE file distributed with this project.
  */
 
+// tslint:disable max-classes-per-file
+
 import { assert } from "chai";
 import { mount } from "enzyme";
 import * as React from "react";
@@ -22,6 +24,23 @@ describe("ContextMenu", () => {
     before(() => assert.isNull(getPopover()));
     afterEach(() => ContextMenu.hide());
 
+    it("Decorator does not mutate the original class", () => {
+        class TestComponent extends React.Component<{}, {}> {
+            public render() {
+                return <div />;
+            }
+
+            public renderContextMenu() {
+                return MENU;
+            }
+        }
+
+        const TargettedTestComponent = ContextMenuTarget(TestComponent);
+
+        // it's not the same Component
+        assert.notStrictEqual(TargettedTestComponent, TestComponent);
+    });
+
     it("React renders ContextMenu", () => {
         ContextMenu.show(MENU, { left: 0, top: 0 });
         assertContextMenuWasRendered();

packages/core/test/hotkeys/hotkeysTests.tsx

@@ -47,6 +47,23 @@ describe("Hotkeys", () => {
         ).to.throw(HOTKEYS_HOTKEY_CHILDREN, "undefined");
     });
 
+    it("Decorator does not mutate the original class", () => {
+        class TestComponent extends React.Component<{}, {}> {
+            public render() {
+                return <div />;
+            }
+
+            public renderHotkeys() {
+                return <Hotkeys />;
+            }
+        }
+
+        const TargettedTestComponent = HotkeysTarget(TestComponent);
+
+        // it's not the same Component
+        expect(TargettedTestComponent).to.not.equal(TestComponent);
+    });
+
     describe("Local/Global @HotkeysTarget", () => {
         let localKeyDownSpy: SinonSpy = null;
         let localKeyUpSpy: SinonSpy = null;