glenjamin / react-hotkey

A simple mixin for application hotkeys
MIT License
35 stars 12 forks source link

Move to es6 and decorators approach. #5

Closed sergey-lapin closed 3 years ago

sergey-lapin commented 9 years ago

Thanks for your project! React 13+ encourage es6 and discourage mixin usage, did you think about moving to decorators instead of mixins?

glenjamin commented 9 years ago

As far as I'm aware, there's no official statement that discourages mixin usage.

Is there a particular problem that being a mixin is causing you?

I would accept a PR that provided a nice higher-order-component API, while maintaining support for the current mixin API.

sergey-lapin commented 9 years ago

Fair enough (about PR). Official statement was at React 0.13 release I guess https://medium.com/@dan_abramov/mixins-are-dead-long-live-higher-order-components-94a0d2f9e750

glenjamin commented 9 years ago

My understanding of that is that while mixin support won't be added to ES6 class component syntax, createClass isn't going away until there's a sensible replacement.

That post provides a nice evaluation of what's good/bad about mixins.

The "good" - attaching onto lifecycle hooks - is exactly what this mixin is doing.

The "bad":

I reckon the main downside of using a mixin here, is that ES6-class-syntax users can't easily consume the library at the moment :imp:

glenjamin commented 9 years ago

Thinking about this even further, this lib could actually expose a component as the API.

It could look like this:

var MyApp = React.createClass({
  onHotkey(e) {
    // do hotkey stuff here
  },
  render() {
    return (
      <div>
        <Hotkey onKey={this.onHotkey} />
        <h1>It's an app!</h1>
      </div>
    );
  }
});

This seems like it might be similar to the ideas in https://github.com/gaearon/react-side-effect

At the moment the app I'm mainly working on is mobile-focused, so I'm not using hotkeys. The previous app that uses hotkeys that I made this lib for is pretty stable at the moment.

I don't really have a big drive to explore these ideas at the moment - but if this is something you're interested in, feel free to submit PRs or build something newer and better :smile: (this is such a small lib, deprecating it for a different one makes more sense than breaking back-compat imo)

neomantra commented 8 years ago

Would you be adverse to a PR that exposes what the Mixin does. but with less magic:

exports.addHandler = function(handler) {
    handlers.push(handler);
};

exports.removeHandler = function(handler) {
    var index = handlers.indexOf(handler);
    if (index >= 0)
        handlers.splice(index, 1);
};

Then somebody with ES6 could easily do this:

import React from 'react';
import hotkey from 'react-hotkey';
hotkey.activate();

class MyComponent extends React.Component {
    constructor() {
        this.hotkeyHandler = this.handleHotkey.bind(this);
    }

    handleHotkey(e) {
        console.log("hotkey", e);
    }

    componentDidMount() {
        hotkey.addHandler(this.hotkeyHandler);
    }

    componentWillUnmount() {
        hotkey.removeHandler(this.hotkeyHandler);
    }
}
glenjamin commented 8 years ago

@neomantra - yep, either that or a HoC while keeping backwards compatibility would be accepted.

havenchyk commented 8 years ago

Hey @glenjamin! what do you think, is there some simple solution to use your mixin as HOC?

I used it with something like HotkeyMixin({'esc': '_handleEsc', 'enter': '_handlerEnter'})

But with mixin I was able to get instance of my Component inside mixin and fire handler. (Like if (event.keyCode === 13) instance[methodName](event))

I don't know how to change this mixin to HOC without pain.

Can you suggest something? Thanks in advance

PS I've checked https://github.com/gaearon/react-side-effect as well, but it wouldn't solve the problem as well.

glenjamin commented 8 years ago

The ES2015 example in the ReadMe shows roughly how to use this with React.Component classes. You can use this approach to write your own higher-order component or a composable component.

I would also accept a PR which added either of these to the project itself.

havenchyk commented 8 years ago

@glenjamin of course, I've checked the readme and I wrote here only because I wasn't able to figure out, how to implement HOC with this approach. I see the only way to implement in every component componentDidMount and componentWillUnmount and add handlers directly there (shared from hoc)

glenjamin commented 8 years ago

Hrm, I just tried it now and a HOC is actually quite tricky - because there's no obvious way to get a reference to the event handler. A composable child component seems to be a better option.

Something like this (untested, just wrote it now in this comment box)

class Hotkey extends React.Component {
    propTypes: {
        onKey: React.PropTypes.func.isRequired
    }
    componentDidMount() {
        hotkey.addHandler(this.props.onKey);
    }
    shouldComponentUpdate(nextProps) {
        return nextProps.onKey !== this.props.onKey;
    }
    componentDidUpdate(prevProps) {
        hotkey.removeHandler(prevProps.onKey);
        hotkey.addHandler(this.props.onKey);
    }
    componentWillUnmount() {
        hotkey.removeHandler(this.props.onKey);
    }
    render() {
        return null;
    }
}

// Use inside your own components:
<div>
  <Hotkey onKey={(e) => this.handleKey(e)} />
</div>
havenchyk commented 8 years ago

because there's no obvious way to get a reference to the event handler

that's exactly what I came against. I tried with the next approach

import hotkey from 'react-hotkey'
import {jwerty} from 'jwerty'

export default (InnerComponent, keysMap) => class extends React.Component {
  constructor() {
    super()

    this.hotkeyHandler = this.hotkeyHandler.bind(this)
    this.ref = this.storeComponentInstanceAfterInitialization.bind(this)
  }

  componentDidMount() {
    hotkey.activate('keydown')
    hotkey.addHandler(this.hotkeyHandler)
  }

  componentWillUnmount() {
    hotkey.removeHandler(this.hotkeyHandler)
  }

  storeComponentInstanceAfterInitialization(innerComponentInstance) {
    this.__instance = innerComponentInstance
  }

  hotkeyHandler(e) {
    Object.keys(keysMap).forEach((keyCode) => {
      const handlerName = keysMap[keyCode]

      if (jwerty.is(keyCode, e)) {
        this.__instance[handlerName](e)
      }
    })
  }

  render() {
    const props = Object.assign(
      {},
      this.props,
      {ref: this.ref}
    )

    return <InnerComponent {...props} />
  }
}

it works, but only if HotkeyHoc is the first wrapper. If you get the result of another HOC wrapping, you will not get access to the methods of instance.

So for me composable child component looks perfect, thank you!

BTW, I don't see any way to implement the same with HOC.

PPS. an approach of https://github.com/glortho/react-keydown looks also good (with decorating)

What do you think?