Inferno: Click events gets called n-times

[aprilmintacpineda]

To see this happening --> https://aprilmintacpineda.github.io/inferno-fluxible/

Note: The same behaviour is not present in react even though they have exactly the same codes --> https://aprilmintacpineda.github.io/react-fluxible/

[aprilmintacpineda]

https://github.com/aprilmintacpineda/inferno-fluxible/blob/master/example/src/Todos.js#L61
https://github.com/aprilmintacpineda/inferno-fluxible/blob/master/example/src/Todos.js#L16

On react:

https://github.com/aprilmintacpineda/react-fluxible/blob/master/example/src/Todos.js#L16
https://github.com/aprilmintacpineda/react-fluxible/blob/master/example/src/Todos.js#L65

[Havunen]

Ok, I need to check this out.

Side note:
Btw inferno-create-class is horrible I don't recommend using it ... its for compatibility reasons there. Better to use Component classes.

[aprilmintacpineda]

Side note: Btw inferno-create-class is horrible I don't recommend using it ... its for compatibility reasons there. Better to use Component classes.

Is the same true for react? We're using inferno-create-class in our company app.

[Havunen]

Well yes because it uses auto bind system, it binds every single method you define on your component on constructor. Seems like React has removed documentation about createClass from their official documentation site, at least I could not find it there. Also there has been deprecation warnings for long time. If you install the package it seems it still depends on React v15

react-create-class package.json

  "devDependencies": {
    "jest": "^19.0.2",
    "prop-types": "^15.5.10",
    "react": "^15.5.4",
    "react-dom": "^15.5.4",
    "webpack": "^2.6.1"
  },

[aprilmintacpineda]

hmmm, how is _auto binding system_ bad? In a class component we'll also bind our methods right?

class MyComponent extends Component {
  constructor (props) {
    super(props);
    this.myMethod = this.myMethod.bind(this);
  }

  // or use an arrow function
  myOtherMethod = () => {
    // stuff
  }
}

I think create class could be an HOC that does the same thing:

const noBindings = [
  '_constructor',
  'constructor',
  'getInitialState',
  'render',
  'componentWillUpdate',
  'componentDidUpdate',
  'componentWillMount',
  'componentDidMount',
  'componentWillUnmount'
  // add more if necessary
];

export default function createClass (componentDefinition) {
  return class extends Component {
    constructor (props) {
      super(props);

      if (componentDefinition.getInitialState) {
        this.state = componentDefinition.getInitialState.call(this);
      }

     if (componentDefinition._constructor) {
       componentDefinition._constructor.call(this);
     }

      const componentMethods = Object.keys(componentDefinition);

      for (let a = 0; a < componentMethods.length; a++) {
        if (noBindings.includes(componentMethods[a])) {
          this[componentMethods[a]] = componentDefinition[componentMethods[a]];
        } else {
          this[componentMethods[a]] = componentDefinition[componentMethods[a]].bind(this);
        }
      }
    }
  }
}

I created this kind of HOC before I learned about inferno-create-class and it worked pretty well. It's not yet very practical to use classes in JavaScript since it's not widely supported yet and we have to transpile our codes for compatibility. I managed to save 5+KB just by transforming our components into not-class components. I think it would be good if we could have a way to create components without using classes.

[Havunen]

Well because it binds everything, performance wise this is non-optimal when constructing the instance.

Imagine you have Button component which has 10 methods, then this button component is used inside list, say 100 rows, 2 buttons each 10 methods; That would be 2000 calls to bind and for in iteration of instance methods of button for 200 times. When possibly there was no bind needed at all.

Some time ago Chrome V8 team optimized bind usage just because of React applications, but if you need to support older browsers it can cause slow down:
http://benediktmeurer.de/2016/01/14/optimizing-bound-functions-further/
http://benediktmeurer.de/2015/12/25/a-new-approach-to-function-prototype-bind/

I'm not sure has Safari, IE, Firefox optimized use case for bind.

Yeah we still need to bind if there is reference to current instance within the instance method. One option is to use linkEvent to pass the parameters you need to the function callback when attaching the event listener to DOM. Sure its not that straightforward because it requires differently structured code.

[aprilmintacpineda]

yeah that makes sense. I guess I could just do this then -->

function Clock () {
  const _this = this;

  _this.state = {
    time: Date.now()
  };

  _this.componentDidMount = function () {
    // update time every second
    _this.timer = setInterval(() => {
      _this.setState({ time: Date.now() });
    }, 1000);
  }

  _this.componentWillUnmount = function () {
    // stop when not renderable
    clearInterval(_this.timer);
  }

  _this.render = function () {
    let time = new Date(_this.state.time).toLocaleTimeString();
    return (
      <div>
      <span>{ 'Inferno version: ' + version }</span>
      <br/>
      <span>{ time }</span>
      </div>
    );
  }

  return _this;
}

Clock.prototype = Component.prototype;
Clock.prototype.constructor = Clock;

https://jsfiddle.net/pzmqLjo7/

[aprilmintacpineda]

@Havunen it's also worth noting that the behaviour described in this issue is only present in production mode.

[Havunen]

@aprilmintacpineda Did you copy correct code, that is the simple clock example?

[aprilmintacpineda]

@Havunen On the function Clock above? Yeah, basically that's the clock example on jsfiddle, and no I did not copy but I transformed it so that it does not use es6 classes.

[Havunen]

There is something really strange going on there, definitely bug somewhere. It might be that its bug in inferno-scripts

[aprilmintacpineda]

@Havunen No it's not. What you said about inferno-create-class was right. I am going to release a new version that does not use inferno-create-class. After not using inferno-create-class, this strange behaviour did not occur anymore.

[aprilmintacpineda]

@Havunen thanks to your helpful insight. That was beautiful. Check out https://aprilmintacpineda.github.io/inferno-fluxible/ it's gone.

[Havunen]

@aprilmintacpineda There is still a bug, I found it... there are multiple instances of InernoJS script attached to your bundle... that is the root cause

[Havunen]

This needs to be fixed in inferno-scripts

[Havunen]

That is also why it works in development mode, but fails in production mode.

[Havunen]

Okay, adding alias to inferno, same way as in development build fixes it.

[Havunen]

removing inferno-create-class seems like fixes it because it depends on Inferno, adding any other package that depends on Inferno would reveal the same bug. Webpack pulls 2 versions of Inferno there, and they confuse each other. Adding alias to inferno seems to fix it so there is only one inferno in production build.

[aprilmintacpineda]

@aprilmintacpineda There is still a bug, I found it... there are multiple instances of InernoJS script attached to your bundle... that is the root cause

Oh no! That means each packages you have that has inferno as a dependency would create different instances of inferno. Did I understood correctly?

[Havunen]

@aprilmintacpineda Yeah, so it seems. its bug. I will update create-inferno-app, I will update latest from the upstream same time.

[Havunen]

The issue is fixed in latest create-inferno-app

[Havunen]

I'm closing this ticket as Inferno v6 is now released! :tada:

https://github.com/infernojs/inferno/releases/tag/v6.0.0