adopted-ember-addons/ember-keyboard

ES6 @keyResponder decorator fails with parameters

toovy opened this issue · 2 comments

toovy commented

Hi,

I'm having issues with a fresh 3.20.0 project. Using the @keyResponder decorator without parameters works, e.g.

@keyResponder
export default class Test extends Component {

Making a simple change like adding the parenthesis, or event adding the priority option leads to an error:

@keyResponder()
export default class Test extends Component {
TypeError: Class extends value undefined is not a constructor or null

There seems to be a difference on how the class decorator is called. When no parenthesis or options are added the call to the decorator declared in https://github.com/adopted-ember-addons/ember-keyboard/blob/master/addon/decorators/key-responder.js#L21 sets the first parameter to the DecoratedClass.

But when adding parenthesis or event options like { priority: 1 } the first parameter is not the decorated class any more, but the options, thus https://github.com/adopted-ember-addons/ember-keyboard/blob/master/addon/decorators/key-responder.js#L30 fails as DecoratedClass is the object { priority: 1 }.

Maybe the way babel transpiles the decorators has changed. This might be the reason why older projects don't have an issue.

I experimented a little bit with the code and came up with a solution that might work for both scenarios. I'm doubting this is backwards compatible. Having little experience with decorators I would like to discuss this change before creating a pull request.

export default function keyResponder(opts = {}) {

  const createClass = function(DecoratedClass) {
    
    if (opts.priority === undefined) {
      opts.priority = 0;
    }
  
    if (opts.activated === undefined) {
      opts.activated = true;
    }

    return class ClassAsKeyResponder extends DecoratedClass {
      static name = `${DecoratedClass.name}WithKeyResponder`;
  
      @service keyboard;
  
      get keyboardPriority() {
        if (super.keyboardPriority === undefined) {
          return opts.priority;
        }
        return super.keyboardPriority;
      }
  
      set keyboardPriority(val) {
        super.keyboardPriority = val;
      }
  
      get keyboardActivated() {
        if (super.keyboardActivated === undefined) {
          return opts.activated;
        }
        return super.keyboardActivated;
      }
  
      set keyboardActivated(val) {
        super.keyboardActivated = val;
      }
  
      constructor() {
        super(...arguments);
        populateKeyboardHandlers(this);
        this.keyboard.register(this);
      }
  
      willDestroy() {
        this.keyboard.unregister(this);
        super.willDestroy(...arguments);
      }
    }
  }

  if (typeof opts === "function") {
    return createClass(opts)
  } else {
    return function(DecoratedClass) {
      return createClass(DecoratedClass)
    }
  }

}

Any thoughts?

BR Tobias

Also found this bug today, would you mind transforming that fix into a PR? Thank in advance!

I'd be happy to review a PR and fast-track a patch release. We will need to make sure that a) we have tests for both variations, and b) tests are running in CI under Ember 3.20.x.