lyonbros / composer.js

Composer is a set of stackable libraries for building complex single-page apps.
http://lyonbros.github.io/composer.js
MIT License
131 stars 9 forks source link

Infinite loop in Controller method: remove() #34

Open mohsentaleb opened 5 years ago

mohsentaleb commented 5 years ago

Consider I'm removing a sub-controller (created with sub), it goes through this function: https://github.com/lyonbros/composer.js/blob/master/src/controller.js#L261 which then releases the controller: https://github.com/lyonbros/composer.js/blob/master/src/controller.js#L264 and then triggers a release event: https://github.com/lyonbros/composer.js/blob/master/src/controller.js#L324 which ultimately runs this line because it's listening to a release event: https://github.com/lyonbros/composer.js/blob/master/src/controller.js#L253 And again goes through remove function. But since it's got a {skip_release: true} option it wont make an infinite loop. I've re-created a scenario here: https://codepen.io/anon/pen/qQaPpL?editors=1011

If you run myDashboard.sub('users').remove('inner') in console everything goes perfectly and removes the inner sub-controller.

The problem is, in my real world application this causes an error Uncaught RangeError: Maximum call stack size exceeded.

If I change this line

instance.bind('release', this.remove.bind(this, name, {skip_release: true}));

to this block, it's fixed:

instance.bind('release', function() {
    this.remove(name, {skip_release: true});
}.bind(this));

Do you have any idea what could be the problem here?

orthecreedence commented 5 years ago

The only diffrence I can see:

In the first version (instance.bind('release', this.remove.bind(this, name, {skip_release: true}))) the event listener uses this.remove at the time of binding the event, not at the time the event is called.

In the second version, the most recent version of this.remove is called, not the version that existed at the time of binding the event.

So, the following would play out like this:

this.remove = function() { console.log('release v1'); };
this.bind('release', this.remove.bind(this, name, {skip_release: true}));
this.bind('release', function() {
    this.remove(name, {skip_release: true});
}.bind(this));

this.trigger('release');
// release v1
// release v1
this.remove = function() { console.log('release v2'); };
this.trigger('release');
// release v1
// release v2
this.remove = function() { console.log('release v3'); };
this.trigger('release');
// release v1
// release v3

My first idea is that this.remove is being overwritten somewhere with a version that doesn't loop endlessly. Are there any references to this.remove in the controller itself?

mohsentaleb commented 5 years ago

Thanks for the explanation, but I'm afraid that's not the case. I put a console.log(name); in composer's remove function and it correctly prints the name of my controller by which I initialized with sub(). That means the correct remove function is called.

remove: function(name, options) {
    options || (options = {});
    if(!this._subcontrollers[name]) return;
    console.log(name);
    if(!options.skip_release) this._subcontrollers[name].release();
    delete this._subcontrollers[name];
}

output:

> App.controllers.pages.currentController.sub('appBarController').remove('billingController')
billingController       composer.js?nocache=872:2299
billingController       composer.js?nocache=872:2299
undefined
orthecreedence commented 5 years ago

Hi, Mohsen. Sorry for the delay in getting back to you.

I've tried quite a bit to reproduce this and I'm not able to. I wonder if you made a simple test case and then bundled it using the same minification process you use in production if it could be reproduced.

Would that be feasible?