[ALOY-1607] Latest underscore version is breaking removeListener method in BaseController.js

GitHub Issue	n/a
Type	Bug
Priority	Critical
Status	Closed
Resolution	Fixed
Resolution Date	2018-03-05T18:23:23.000+0000
Affected Version/s	n/a
Fix Version/s	CLI Release 7.0.3
Components	Tooling
Labels	n/a
Reporter	Jorge Macias Garcia
Assignee	Feon Sua Xin Miao
Created	2018-03-01T13:36:55.000+0000
Updated	2018-04-11T21:33:01.000+0000

Description

Latest underscore version (1.8.3) used in Alloy 1.11.0 causes an error using *$.removeListener* method. https://github.com/appcelerator/alloy/blob/2a25b1d50fe22d3f5c4f423a6027e1c1d7748b88/Alloy/lib/alloy/controllers/BaseController.js#L496 I don't know why Appc Team use underscore inside BaseController.js instead use built in prototype functions like 'forEach'. In fact bump to latest underscore version is breaking the normal usage of $.removeListenermethod and this is why: BaseController.js


removeListener: function(proxy, type, callback) {
  _.each(this.__events, function(event, index) {
    if ((!proxy || proxy.id === event.id) &&
      (!type || type === event.type) &&
      (!callback || callback === event.handler)) {
      event.view.removeEventListener(event.type, event.handler);

      //This line is removing a item from __events array but a undefined value is placed in the removed index
      //self.__events.splice(index, 1); removes the item with no undefined value instead
      delete self.__events[index];
    }
  });
  return this;
}

What happens in the underscore 1.8.3 version *_.each* method? Well, it iterates in the array using a *for*, undefined values included. If *event* is undefined when *event.view.removeEventListener(event.type, event.handler);* it fails because *view* isn't in event {color:red}[ERROR] Error catched: Cannot read property 'view' of undefined{color}

_.each = _.forEach = function(obj, iteratee, context) {
  iteratee = optimizeCb(iteratee, context);
  var i, length;
  if (isArrayLike(obj)) {
    for (i = 0, length = obj.length; i < length; i++) {
      iteratee(obj[i], i, obj);
    }
  } else {
    var keys = _.keys(obj);
    for (i = 0, length = keys.length; i < length; i++) {
      iteratee(obj[keys[i]], keys[i], obj);
    }
  }
  return obj;
};

What happens in the oldest underscore 1.6.0 version *_.each* method? Well, it checks if *forEach* method is present in Array.prototype and uses it. This method skipes undefinedvalues.

var ArrayProto = Array.prototype;
var nativeForEach      = ArrayProto.forEach;

var each = _.each = _.forEach = function(obj, iterator, context) {
    if (obj == null) return obj;
    if (nativeForEach && obj.forEach === nativeForEach) {
      obj.forEach(iterator, context);
    } else if (obj.length === +obj.length) {
      for (var i = 0, length = obj.length; i < length; i++) {
        if (iterator.call(context, obj[i], i, obj) === breaker) return;
      }
    } else {
      var keys = _.keys(obj);
      for (var i = 0, length = keys.length; i < length; i++) {
        if (iterator.call(context, obj[keys[i]], keys[i], obj) === breaker) return;
      }
    }
    return obj;
  };

Use case in Titanium:

    $.addListener($.win, 'open', disableClick);
    $.addListener($.win, 'close', clean);
    $.addListener($.disable, 'click', disableClick);
    $.addListener($.label, 'click', close);

function disableClick() {
    //Removes listener but undefined value is placed
    $.removeListener($.disable, 'click', disableClick);
  }

  function close(e) {
    Ti.API.info('Closing window');
    $.win.close();
  }

  function clean(e) {
    Ti.API.info('Cleaning window');
    try {
      Ti.API.info('Call $.removeListener()');
      $.removeListener();
    } catch (err) {
      //It tries to remove event to undefined value
      //[ERROR] Error catched: Cannot read property 'view' of undefined
      Ti.API.error('Error catched: ' + err.message);
    }
  }

How to solve: How you can solve this? So easy... First way: Change _.each for forEach

removeListener: function(proxy, type, callback) {
  this.__events.forEach(function(event, index) {
    if ((!proxy || proxy.id === event.id) &&
      (!type || type === event.type) &&
      (!callback || callback === event.handler)) {
      event.view.removeEventListener(event.type, event.handler);
      delete self.__events[index];
    }
  });
  return this;
}

Second way: Adding a simple check in the if...

removeListener: function(proxy, type, callback) {
  _.each(this.__events, function(event, index) {
    if (event && (!proxy || proxy.id === event.id) &&
      (!type || type === event.type) &&
      (!callback || callback === event.handler)) {
      event.view.removeEventListener(event.type, event.handler);
      delete self.__events[index];
    }
  });
  return this;
}

As Konstantin Bueschel commented a way to avoid this undefined holes inside arrays is using *splice* instead *delete* *self.__events.splice(index, 1);*

Attachments

File	Date	Size
app-removelistener.zip	2018-03-01T13:43:07.000+0000	9206826

Comments

Konstantin Bueschel 2018-03-01

Hei, in my opinion the root cause, is the delete self.__events[index]; statement, cause it places a _null_ value at the given index. Therefore it should be replaced with the self.__events.splice(index, 1); Besides that, I agree with Jorge, that there's not need for the underscore __.each_ usage. It would also remove the dependency on underscore and the undefined check of the iteration item. Thanx Jorge for filing this issue.
Jorge Macias Garcia 2018-03-01

Yes, you are right splice removes a item in the array and not fills its position with undefined value. The solution could be both use prototype forEach and splice instead delete.
Jorge Macias Garcia 2018-03-02

PR - [https://github.com/appcelerator/alloy/pull/881](https://github.com/appcelerator/alloy/pull/881)
Lokesh Choudhary 2018-04-11

Verified the fix in alloy 1.12.0 in core 7.0.3-master.36. Closing.

JSON Source