X.on now always allocates an array - tidies up code (fix espruino#2559)

Author: gfwilliams

ChangeLog

@@ -37,6 +37,7 @@
             Network: enable parsing of 'https' URL and TLS flag even even TLS not built in (#2410)
             removeListener while executing events no longer stops subsequent listeners from executing (#2151)
             jsvObjectIterator is now safe even if not called on something iterable
+            X.on now always allocates an array - tidies up code (fix #2559)
 
      2v24 : Bangle.js2: Add 'Bangle.touchRd()', 'Bangle.touchWr()'
             Bangle.js2: After Bangle.showTestScreen, put Bangle.js into a hard off state (not soft off)

src/jswrap_object.c

@@ -19,6 +19,7 @@
 #include "jswrapper.h"
 #include "jswrap_stream.h"
 #include "jswrap_functions.h"
+#include "jswrap_array.h" // for splice
 #ifdef __MINGW32__
 #include "malloc.h" // needed for alloca
 #endif//__MINGW32__
@@ -889,23 +890,22 @@ void jswrap_object_on_X(JsVar *parent, JsVar *event, JsVar *listener, bool addFi
   JsVar *eventList = jsvFindChildFromVar(parent, eventName, true);
   jsvUnLock(eventName);
   JsVar *eventListeners = jsvSkipName(eventList);
+  // if no array, add it
   if (jsvIsUndefined(eventListeners)) {
-    // just add the one handler on its own
-    jsvSetValueOfName(eventList, listener);
-  } else {
-    // we already have an array and we just add to it
-    // OR it's not an array but we need to make it an array
-    JsVar *arr = jsvNewEmptyArray();
-    if (addFirst) jsvArrayPush(arr, listener);
-    if (jsvIsArray(eventListeners))
-      jsvArrayPushAll(arr, eventListeners, false);
-    else
-      jsvArrayPush(arr, eventListeners);
-    if (!addFirst) jsvArrayPush(arr, listener);
-    jsvSetValueOfName(eventList, arr);
-    jsvUnLock(arr);
+    eventListeners = jsvNewEmptyArray();
+    jsvSetValueOfName(eventList, eventListeners);
+  }
+  jsvUnLock(eventList);
+  if (!eventListeners) return; // out of memory
+  // now have an array and we just add to it
+  if (addFirst) { // add it first?
+    JsVar *elements = jsvNewArray(&listener, 1);
+    jswrap_array_unshift(eventListeners, elements);
+    jsvUnLock(elements);
+  } else { // or add it at the end
+    jsvArrayPush(eventListeners, listener);
   }
-  jsvUnLock2(eventListeners, eventList);
+  jsvUnLock(eventListeners);
   /* Special case if we're a data listener and data has already arrived then
    * we queue an event immediately. */
   if (jsvIsStringEqual(event, "data")) {
@@ -1033,16 +1033,12 @@ void jswrap_object_removeListener(JsVar *parent, JsVar *event, JsVar *callback)
     jsvUnLock(eventName);
     JsVar *eventList = jsvSkipName(eventListName);
     if (eventList) {
-      if (eventList == callback) {
-        // there's no array, it was a single item
-        jsvRemoveChild(parent, eventListName);
-      } else if (jsvIsArray(eventList)) {
+      if (jsvIsArray(eventList)) {
         // it's an array, search for the index
         JsVar *idx = jsvGetIndexOf(eventList, callback, true);
-        if (idx) {
+        if (idx)
           jsvRemoveChildAndUnLock(eventList, idx);
-        }
-      }
+      } // otherwise something is wrong, but lets just ignore it
       jsvUnLock(eventList);
     }
     jsvUnLock(eventListName);

tests/test_eventemitter_removeListener.js

@@ -8,9 +8,12 @@ var a = {}; // should be EventEmitter, but currently all objects have it
 
 a.on("data", foo);
 a.emit("data", 8);
-a.removeListener("data", foo);
-a.emit("data", 9);
-
+// we need a delay, because events are processed in the next idle loop
+// and if we remove the listener before then, it won't get the event
 setTimeout(function() {
-  result = x==42;
+  a.removeListener("data", foo);
+  a.emit("data", 9);
 }, 1);
+setTimeout(function() {
+  result = x==42;
+}, 2);