Hotkeys component results in TypeError with ES6 target

[sushain97]

N.B. this is a more actionable version of #2535 with repro links.

Environment

• Package version(s): 3.6.1
• Browser and OS versions: MacOS Chrome 68

Steps to reproduce

1. Use target: es6 in tsconfig.json.
2. Add a @HotkeysTarget and associated code as the docs describe.

Rationale

Per @giladgray

targeting es5 is a good practice for browser apps as es6+ is inconsistently supported.

In general, this makes sense to me. However, I would consider apps primarily intended for personal/internal use where modern browsers are implied as an exception (or Electron apps). Forwards compatibility also seems like a good way to 'future proof'.

Actual behavior

See https://codesandbox.io/s/7yp63vw621:

TypeError: Class constructor App cannot be invoked without 'new'
    at new HotkeysTargetClass (https://7yp63vw621.codesandbox.io/node_modules/@blueprintjs/core/lib/cjs/components/hotkeys/hotkeysTarget.js:20:50)
    at constructClassInstance (https://7yp63vw621.codesandbox.io/node_modules/react-dom/cjs/react-dom.development.js:6789:20)
    at updateClassComponent (https://7yp63vw621.codesandbox.io/node_modules/react-dom/cjs/react-dom.development.js:8324:9)
    at beginWork (https://7yp63vw621.codesandbox.io/node_modules/react-dom/cjs/react-dom.development.js:8966:16)
    <...snip...>

I think this breakage will also affect other decorators.

Expected behavior

Hotkeys work as expected.

See https://codesandbox.io/s/q3jk1n66mj.

Possible solution

I tried using the following to get around this but didn't have much luck:

function WrappedApp(props: IAppProps) {
  return new App(props);
}
WrappedApp.prototype = Object.create(App.prototype);

const AppWithHotkeys = HotkeysTarget(WrappedApp as any);
ReactDOM.render(<AppWithHotkeys {...(window as any).CONTEXT} />, document.getElementById('app'));

The render call from HotkeysTarget still never gets called.

[sumit-sinha]

I tried a similar workaround as you but it worked for me, the only change I made was inside the function where I am not doing anything

function AppWrapper() {} // tslint:disable-line
AppWrapper.prototype = Object.create(App.prototype);
AppWrapper.prototype.renderHotkeys = function() {
  return (
    <Hotkeys>
       <Hotkey global={true} combo="ctrl+s" label="Save" />
    </Hotkeys>
  );
};

export const AppContainer = HotkeysTarget(AppWrapper as any);

[sushain97]

@sumit-sinha are you by any chance wrapping a component that doesn't use state (i.e. never accesses this.state) or a functional component?

When I try using your approach, I run into null property accesses in my wrapped component's render method since this.state is null.

[sumit-sinha]

@sushain97 yes I tried with a stateless component

[sushain97]

@sumit-sinha ah.

Here's how I modified your solution a bit to work for stateful apps and support an arbitrary number of hotkeys with callbacks that use the state while maintaining some type safety: sushain97/web2fs-notepad@63e0dbf.

[sumit-sinha]

@sushain97 yeah wrapping a stateful component in a stateless component seems to work well

quick recap

class StatelessApp extends React.PureComponent {
  // tslint:disable-line max-classes-per-file
  public render() {
    return <App {...{ ...context, settings, noteSettings, hotkeyCallbacks }} />;
  }
}

function AppWrapper() {} // tslint:disable-line no-empty
AppWrapper.prototype = Object.create(StatelessApp.prototype);
AppWrapper.prototype.renderHotkeys = App.hotkeyRenderer(hotkeyCallbacks);
export const AppContainer = HotkeysTarget(AppWrapper as any);

[giladgray]

@sushain97 see #1725: Hotkeys only work if value returned from render is a DOM element.

this is a known issue and a significant limitation of our decorator APIs. 🤷‍♂️

[biels]

Is this going to be addressed soon?

[giladgray]

@biels no, as a proper fix likely requires an API break (and a major version) which we are not prepared to develop at the moment.

[andyfleming]

The simpler solution for me was to just use react-hotkeys. The decorator was breaking the testability of my app and the options for workarounds don't seem worth it.

[adidahiya]

Fixed by migrating to HotkeysTarget2 or useHotkeys