Applying an addEventListener in a loop

Question

This is my first question ever thus I apologize in advance might I use the wrong netiquette.

I'm exploring different solutions to implement a two way binding using Javascript only, I ran across the 'common mistake' when using a Closure inside a for loop hence having the counter variable always set on the last item, I found the explanation (and the solution) on this very site but then I came across a different issue I'd appreciate some help for.

Imagine we have two sets of data, where one contains proper data, i.e.:

   var data = {
      data1 : 0
   };

The other a collection of objects describing 3 elements :

  var elements = {

    1 : {
      target  : 'main',
      value   : data,
      element : 'div',
      events  : {
        click : 'add'
      }
    },

    2 : {
      target  : 'main',
      value   : data,
      element : 'div',
      events  : {
        click : 'add'
      }
    },

    3 : {
      target  : 'main',
      value   : data,
      element : 'div',
      events  : {
        click : 'add'
      }
    }

  }

See the complete codesnippet below

var data = {
        data1 : 0
      };
      
      var elements = {
      
        1 : {
          target  : 'main',
          value   : data,
          element : 'div',
          events  : {
            click : 'add'
          }
        },
        
        2 : {
          target  : 'main',
          value   : data,
          element : 'div',
          events  : {
            click : 'add'
          }
        },
        
        3 : {
          target  : 'main',
          value   : data,
          element : 'div',
          events  : {
            click : 'add'
          }
        }
      
      }
      

      // This is our main object, we define the properties only ...
      var _elem = function (props,id){

        this.id      = id;
        this.target  = document.getElementById(props.target);
        this.element = document.createElement(props.element);
        this.events  = props.events;
        this.value   = props.value;
        
      }

      // Then we add a method to render it on the page ... 
      _elem.prototype.render = function(){
        // I added the Object Id for debugging purposes
        this.element.innerHTML = this.value.data1 + ' ['+this.id+']';
        this.target.appendChild(this.element);
      }

      // ... and another to change the underlying data and re - render it
      _elem.prototype.add = function(){
        // Since the data is a reference to the same data object
        // We expect to change the value for all the elements
        this.value.data1++;  
        this.render();
      }
      
      // First we looop trough the array with the element definition and
      // Cast each item into a new element
      for(var el in elements){
        elements[el] = new _elem(elements[el],el);
      }

      
      // Then we apply the event listener (if any event description is present)
      for(var el in elements){

        if(!elements[el].hasOwnProperty( 'events' )){
          continue;
        } 
      
        // We use the anonymous function here to avoid the "common mistake"
        (function() {  
        
          var obj    = elements[el];    
          var events = obj.events;    
          
          for(var ev in events){
             obj.element.addEventListener(ev,function(){ obj[events[ev]]() });
          }
        
        })();    
        
      }
      
      // And finally we render all the elements on the page  
      for(var el in elements){
         elements[el].render(elements[el]);
      }

div {
  padding: 10px;
  border: solid 1px black;
  margin: 5px;
  display: inline-block;
}

<html>

<head>
</head>

<body>
  <div id="main"></div>
</body>

</html>

Now, if we click button [1] it will update itself and the following, resulting in this sequence:

0 [2] 0 [3] 1 [1]

We refresh the page and this time click button [2], the sequence will be:

0 [1] 0 [3] 1 [2]

Button [3] Instead will update itself only

0 [1] 0 [2] 1 [3]

I did look for this topic before posting but all I could find were questions similar to this: addEventListener using for loop and passing values , where the issue was the counter variable holding always the last value

In this case instead it seems the issue to be the opposite, or the object holding the initial value and the ones following (if you keep clicking you will see what I mean)

What am I do wrong?

You have implicit global variables all over the place. Fix those before you consider any other problems. — , Mar 14 '17 at 17:13
...the "closure in a loop" issue is definitely a problem in your code. I don't know how it relates to your description, but it seems that you know about that issue already, so why hasn't it been fixed? How do you know that's not a contributing factor? — , Mar 14 '17 at 17:16
So ultimately, I'm almost positive that the issue is a combination of those two things. If you implicitly declare global variables, your IIFE doesn't help at all. And then the closure in a loop problem that you already know about suffers additionally because of the globals. — , Mar 14 '17 at 17:20
@squint, in the event listener loop I enclosed the "event" loop inside an anonymous function just to avoid the issue, otherwise clicking on any button would update only the last one. Btw which implicit global variables are you referring to? — maldi, Mar 14 '17 at 17:24
I'm still not understanding the problem. You are changing the order of your elements each time, and only the item you clicked on is updated. There's no difference between items 1, 2 and 3. — JDB, Mar 14 '17 at 17:26
`for (el in...` and `for (ev in...` create globals; there's no `var`, `let` or `const`. Your IIFE encloses code and creates variables within, but then you have another loop inside that, which is where the handler is created. That's where the IIFE would need to go. — , Mar 14 '17 at 17:27
@JDB, "You are changing the order of your elements" oooopsss, you're right, should I delete this? — maldi, Mar 14 '17 at 17:28
I don't know... is that not what you want? I don't know what your end goal was. — JDB, Mar 14 '17 at 17:29
@squint, I see what you mean, but still declaring them won't change the behaviour — maldi, Mar 14 '17 at 17:30
`_elem = function (props,id){` also creates a global. The implicit globals should be fixed, but I thought I saw one used in the handler, which isn't the case. The "closure in a loop" is a problem because of `var func = events[ev];` in the innermost loop. — , Mar 14 '17 at 17:31
...actually, you could get by without another IIFE, if you change this: `obj.element.addEventListener(ev,function(){ obj[func]() });` to this: `obj.element.addEventListener(ev, obj[func]);` ...because that will resolve the value of `obj[func]` immediately instead of when the click occurs. — , Mar 14 '17 at 17:34
Frankly, I'd probably combine the first two loops, and reduce it all down to this: https://jsfiddle.net/mrhwm73w/ — , Mar 14 '17 at 17:40
@squint I fixed the global vars and the function in the inner looop but still no joy, also obj.element.addEventListener(ev, obj[func]); won't work — maldi, Mar 14 '17 at 17:40
@JDB, my final goal would be map other elements to the one element's event, but in order to do that I need to create the elements first, that's the reason why I do 3 loops — maldi, Mar 14 '17 at 17:42
@squint becasue I tried it locally and gives me this : dbind.html:75 Uncaught TypeError: Cannot read property 'data1' of undefined at HTMLDivElement._elem.add (dbind.html:75) — maldi, Mar 14 '17 at 17:44
3 loops is fine, but you need to differentiate between an initial render and subsequent refreshed. Also, a child should generally not be responsible for appending itself to a parent - makes refactoring much more difficult later on. — JDB, Mar 14 '17 at 17:45
I believe JDB gave the correct answer : the issue is visual since the button will always be rendered at the bottom, how do assign the correct answer to him? — maldi, Mar 14 '17 at 17:48
Oh, I see. You're expecting `this` to be your object. Using `.bind()` would solve that. Strangely, your first code example uses `var func = ...` and the second does not. So the second doesn't have the closure in a loop problem but the first does. — , Mar 14 '17 at 17:52
...I guess you updated one and not the other. So you fixed that issue, and now you're down to the behavior problem, which JDB solved. — , Mar 14 '17 at 17:54
...actually, you still have a closure/loop problem with your fix. `obj[events[ev]]()`. You're now using `ev`, which all handlers share. — , Mar 14 '17 at 17:56
Just so you understand, the `var` you just added only helps because each `events` object only has one item in it. As soon as more are added, you're back to the closure/loop problem. — , Mar 14 '17 at 18:14

score 0 · Accepted Answer · answered Mar 14 '17 at 18:03

The issue appears to be that you are re-appending your child elements on each "refresh", which shifts the order of the elements and gives the illusion of refreshing multiple elements.

You need to differentiate between an initial render and subsequent refreshes.

I recommend that you remove the append from your render function and instead handle appending in your final for loop:

// And finally we render all the elements on the page  
for(el in elements){
  elements[el].render(elements[el]);
  elements[el].target.append(elements[el].element);
}

Note that there are multiple "issues" with your code, including global variables in several locations. And I'm not confident that your architecture will scale well. But, those issues are outside the scope of your question, and you'll learn as you go... no reason to expect that everyone will know everything, and you may find that your current solution works just fine for what you need it to do.

var data = {
        data1 : 0
      };
      
      var elements = {
      
        1 : {
          target  : 'main',
          value   : data,
          element : 'div',
          events  : {
            click : 'add'
          }
        },
        
        2 : {
          target  : 'main',
          value   : data,
          element : 'div',
          events  : {
            click : 'add'
          }
        },
        
        3 : {
          target  : 'main',
          value   : data,
          element : 'div',
          events  : {
            click : 'add'
          }
        }
      
      }
      

      // This is our main object, we define the properties only ...
      var _elem = function (props,id){

        this.id      = id;
        this.target  = document.getElementById(props.target);
        this.element = document.createElement(props.element);
        this.events  = props.events;
        this.value   = props.value;
        
      }

      // Then we add a method to render it on the page ... 
      _elem.prototype.render = function(){
        // I added the Object Id for debugging purposes
        this.element.innerHTML = this.value.data1 + ' ['+this.id+']';
      }

      // ... and another to change the underlying data and re - render it
      _elem.prototype.add = function(){
        // Since the data is a reference to the same data object
        // We expect to change the value for all the elements
        this.value.data1++;  
        this.render();
      }
      
      // First we looop trough the array with the element definition and
      // Cast each item into a new element
      for(var el in elements){
        elements[el] = new _elem(elements[el],el);
      }

      
      // Then we apply the event listener (if any event description is present)
      for(var el in elements){

        if(!elements[el].hasOwnProperty( 'events' )){
          continue;
        } 
      
        // We use the anonymous function here to avoid the "common mistake"
        (function() {  
        
          var obj    = elements[el];    
          var events = obj.events;    
          
          for(ev in events){
             obj.element.addEventListener(ev,function(){ obj[events[ev]]() });
          }
        
        })();    
        
      }
      
      // And finally we render all the elements on the page  
      for(var el in elements){
         elements[el].render(elements[el]);
         elements[el].target.appendChild(elements[el].element);
      }

div {
  padding: 10px;
  border: solid 1px black;
  margin: 5px;
  display: inline-block;
}

<html>

<head>
</head>

<body>
  <div id="main"></div>
</body>

</html>

@maldi - since [you asked](http://stackoverflow.com/questions/42792321/applying-an-addeventlistener-in-a-loop#comment72700716_42792321), you can upvote and/or "check" this answer to indicate the it was useful/the most helpful answer. See [What should I do when someone answers my question?](http://stackoverflow.com/help/someone-answers) — JDB, Mar 14 '17 at 18:13
@maldi - However, you should *never* feel pressured to do either of those things. What you do with the answers to your questions is entirely up to you. You are not required to vote on or accept any answer. — JDB, Mar 14 '17 at 18:14
Well, you pointed me to the real issue, that being visual more than functional -although I agree the code had issues itself- I would probably have applied different styles in the end but you saved me a good deal of time. — maldi, Mar 14 '17 at 18:15

Applying an addEventListener in a loop

1 Answers1