Component/Mixin functions not bound to component scope

[josterholt]

Main Issue When using React-Stampit with Cerebral, I get the error: Cannot read property 'cerebral' of undefined (Error is on line 39 of cerebral/src/mixin.js)

Stack Trace

mixin._mergeInState @ mixin.js:39
EventEmitter.emit @ events.js:99
(anonymous function) @ createSignalMethod.js:169
(anonymous function) @ createSignalMethod.js:165
signalPath.path.(anonymous function).runSignal @ createSignalMethod.js:181

This is due to the this keyword referencing Cerebral and not the component.

After debugging this I found that React auto-binds the component scope for components/mixins, so the "this" keyword references the component.

Example code can be found here. Here's my react stamp, and then the main app file.

Secondary Issue A related issue, is events in component markup require being wrapped in a function in components because they too are lacking the component scope. They do not need to be wrapped when using React.createClass.

Example:

export default React => stampit(React, {
    ...
    onBtnPress() {
        this.signals.addTodo();
    },
    render() {
        ...
        return (
            ...
            <button onClick={ () => this.onBtnPress() }>Press Me</button>
            ...
        );
    }
})
.compose(mixin);

Where changing the onClick attribute on <button /> will affect the reference of this in onBtnPress.

    onClick={ this.onBtnPress } // In onBtnPress: this = Window
    onClick={ () => this.onBtnPress() } // In onBtnPress: this = Component

When using React.createClass, no wrapping is needed.

Findings I tried finding a fix for this myself, but I'm having trouble finding where the instance of my component is being held/created. In react-stampit compose function, I only have access to the factory. Haven't been able to get passed this point yet, but will keep trying. (Maybe someone can help point me in the right direction.)

I believe this can be fixed by using .bind(componentInstance) on each component method. I've been able to hack the compose function so that it does bind something, however it's not the component instance, so no dice.

It looks like React has an autobinding feature which is used in the function mixSpecIntoComponent in ReactClass.js.

if (shouldAutoBind) {
    if (!proto.__reactAutoBindMap) {
        proto.__reactAutoBindMap = {};
    }
    proto.__reactAutoBindMap[name] = property;
    proto[name] = property;
} else {
    ...
}

I don't know what any of the variables involved in this code block really are. Below is based off of what I've observed:

proto is ReactClass.createClass.Constructor name is getDOMNode or trapBubbledEvent property is a Mixin (ReactBrowserComponentMixin or LocalEventTrapMixin)

I can only guess how to fit this into React-Stampit at the moment. I will try accessing the constructor via Factory.constructor, and go from there.

[ericelliott]

What line do you get the error on? If possible, please supply the full stack trace.

[ericelliott]

A related issue, is events in component markup require being wrapped in a function in components because they too are lacking the component scope.

Yes, you absolutely should use arrow functions for react event listeners:

onClick={ () => this.onBtnPress() }

[josterholt]

Updated original post to better reflect the issues, give clarification, and describe what I've found.

[troutowicz]

Neither of these reports are bugs in react-stampit.

• cannot read property 'cerebral' of undefined

React 0.13 does not support using childContext with factories. This has been patched here and should be fixed in the React 0.14 beta.

• events in component markup require being wrapped in a function

Correct. This was a design decision, as mentioned in the README.

[ericelliott]

@Shroder Feel free to leave this open until you're satisfied.

If you're satisfied now, feel free to close. =)

[josterholt]

Thanks for the response @troutowicz.

Regarding the patch you linked, I think we may be talking about two different things. It looks like the patch would fix an issue with context, but not the scope available in the component functions, which is the issue I'm reporting.

I have the beginning of a patch which fixes issues reported in this thread. Implementation will be similar to what React does in the ReactClass(mixSpecIntoComponent).

(Note: I'll update with ES6 code once I get that compiled and prove it to be working)

ES5 Full JavaScript Function

function compose() {
  for (var _len = arguments.length, stamps = Array(_len), _key = 0; _key < _len; _key++) {
    stamps[_key] = arguments[_key];
  }

  var result = (0, _stampit2['default'])(),
      refs = { state: {} },
      init = [],
      methods = {},
      statics = {};

  if ((0, _utils.isStamp)(this)) {
    stamps.push(this);
  }

  (0, _lodashCollectionForEach2['default'])(stamps, function (stamp) {
    stamp = !(0, _utils.isStamp)(stamp) ? rStampit(null, stamp) : stamp; // eslint-disable-line

    init = init.concat(stamp.fixed.init);
    methods = wrapMethods(methods, stamp.fixed.methods);
    statics = extractStatics(statics, (0, _lodashObjectOmit2['default'])(stamp, function (val, key) {
      return (0, _lodashObjectHas2['default'])(result, key);
    }));

    if (stamp.fixed.refs && stamp.fixed.refs.state) {
      (0, _lodashObjectAssign2['default'])(refs.state, stamp.fixed.refs.state, dupeFilter);
    }
  });

  result = result.init(init).refs(refs).methods(methods)['static'](statics);
  result.compose = compose;

  /**
   * MODIFIED CODE
   * Bind instances to methods
   */
  result = result.init(function (options) {
    var instance = options.instance;
    var fixed = options.stamp.fixed;
    methods = fixed.methods;
    for (var name in methods) {
      /**
       * This is temporary. Need to add validation/check code
       * to filter out methods that shouldn't be bound.
       */
      if(name == '_mergeInState' || name == 'onBtnPress') { 
        instance[name] = methods[name].bind(instance);  
      }
    } 
  });
  /*
   * END MODIFICATION
   */  

  return (0, _utils.stripStamp)(result);
}

With that the cerebral mixin now works, and I'm able to code the event listener without wrapping it in a function, just like I am with the basic React framework:

<button onClick={ this.onBtnPress }>Press Me</button>

I need to understand StampIt better before releasing an actual patch. Maybe this is actually incompatible with how StampIt is meant to be used? (Maybe @troutowicz or @ericelliott can answer this for me)

Caveats/considerations: 1.) Handling methods that cannot be bound 2.) Only binding a method once 3.) Rebinding when instance changes(?) 4.) Applying patch to other methods that produce a stamp

[troutowicz]

We are talking about the same thing. You reported the undefined error occurs at line 39 of Cerebral's mixin. That line is referencing context.

Hmm, I'm not sure you read my response. We do not want this library to perform magical autobinding like what is observed in React.createClass. I would suggest not spending time on a patch until a bug has actually been confirmed. :)

[ericelliott]

@Shroder If I'm reading everything right, this behavior is intentional.

We do not want this library to perform magical autobinding like what is observed in React.createClass

:+1:

[josterholt]

Ok, gotcha. Where can I read up on where React is going? Seems like referencing the current design is not the way to go.

I will try the patch and see if that gets me by. Thanks for the info/clarification!

[troutowicz]

The React blog highlights the major changes of v0.14.

https://facebook.github.io/react/blog/2015/07/03/react-v0.14-beta-1.html

[ericelliott]

Yep. We should probably make any necessary changes for 0.14 and prepare a major version bump if we have any breaking changes.

[troutowicz]

Closing this for now, reopen if more discussion is required.