mbrowne / bound-decorator Goto Github PK

Using an initializer to bind the method essentially converts the method into a field, which has all of the same problems as laid out in the rationale doc:

class Counter extends React.Component {
    ...
    @bound
    handleClick() {
        this.setState({ count: this.state.count + 1 })
    }
    ...
}

class SpecialCounter extends Counter {
    handleClick() {
        console.log("SpecialCounter clicked")
    }
    ...
}

const specialCounter = new SpecialCounter()
// Which method gets called?
specialCounter.handleClick()
// (Note: this example is for illustrative purposes only; obviously we wouldn't really call handleClick() manually)

An additional constraint worth considering is that binding on both the superclass and the subclass should still Just Work ™️, even with calls to super:

class Counter extends React.Component {
    ...
    @bound
    handleClick() {
        this.setState({ count: this.state.count + 1 })
    }
    ...
}

class SpecialCounter extends Counter {
    @bound
    handleClick() {
        super.handleClick();
        // do more things
    }
    ...
}

Currently, super.handleClick() will not exist on the superclass since it is a class field, and it will be overwritten by the time it gets to the child. This is also problematic for the strategy of dynamically redefining the method within a getter, since the call to super will then overwrite the subclass's version.

The best strategy we have found to mitigate this is to use a WeakMap to map bound functions to the instance, and to bind the function in a getter:

const BINDINGS_MAP = new WeakMap();

export function bound(desc) {
  let boundFn = desc.descriptor.value;

  desc.descriptor = {
    get() {
      let bindings = BINDINGS_MAP.get(this);

      if (bindings === undefined) {
        bindings = new Map();
        BINDINGS_MAP.set(this, bindings);
      }

      let fn = bindings.get(boundFn);

      if (fn === undefined) {
        fn = boundFn.bind(this);
        bindings.set(boundFn, fn);
      }

      return fn;
    }
  }

  return desc;
}

There's currently no way to test how this decorator works with private methods because Babel hasn't implemented private methods/fields for decorators yet, but I am starting to think about how they should be handled... I realized that the current implementation doesn't make sense for private methods because it always returns a new bound method using extras, but in the case of private methods it should just replace the existing method since it didn't exist on the prototype in the first place.

Also, just creating a new private (bound) method doesn't fully serve the goals of making objects testable. Most build tools these days as well as node.js make process.env.NODE_ENV available, so one idea would be to do something like this to check if the app is running in a 'test' environment:

And then for the test environment, the behavior would be to:

1. Replace the original private method with a private bound method (this should happen for all environments)
2. Move the original private method to the public prototype (this should happen only in test environments). Obviously this would require changing the key to a string since using the existing key (private name) would mean the method is still private.

There could also be a parameter to the decorator allowing the user to customize this behavior, e.g.:

privateMethodHandling: 'basedOnEnvironment' | 'alwaysMakePublic'
(default: 'basedOnEnvironment')