Functions declared within loops referencing an outer scoped variable may lead to confusing

Question

This is a jshint warning question.How can I solve this problem?

var comment_btn=document.querySelector('.comment_button');
var comment_ul=document.querySelector('.comment_ul');
var comment_text=document.querySelector('#comment');

comment_btn.onclick = function(){
    var comment_li = document.createElement('li');
    comment_li.className = 'comment_li';
    if(comment_text.value != '') {
        comment_li.innerHTML = comment_text.value + "<a class='comment_a' href='javascript:;'>Delete</a>";
        comment_ul.insertBefore(comment_li,comment_ul.children[0]);
        var del = document.querySelectorAll('.comment_a');
        for (var i = 0; i < del.length; i++) {
            del[i].onclick = function() {
                comment_ul.removeChild(this.parentNode);
            };
        }
    }
    else {
        alert('Please input!');
    }
};

Warning:
Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. (comment_ul) (W083)jshint(W083)

I really can't think of a solution,please help me.

See if the following explanation helps, JS documentation, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/var — MZM, Nov 30 '22 at 00:14
`var del = document.querySelectorAll('.comment_a');` is going to select every element on the page. So if you click `comment_btn` more than once you will be adding a lot of events. You should only be selecting the `a` in the li you create. You should not have to loop at all. — epascarello, Nov 30 '22 at 00:17

score 0 · Answer 1 · answered Nov 30 '22 at 00:15

The function you assign to onclick inside the loop doesn't care about any value that changes during the course of the loop. It only uses comment_url (defined before the loop) and this.

Create the function before the loop and assign it to a variable.

Copy it to onclick inside the loop.

score 0 · Answer 2 · answered Nov 30 '22 at 00:20

This is a warning which will appear if you have a loop and you have a variable declared with var, which can sometimes cause problems. Assuming you aren't reassigning comment_ul anywhere, the problems aren't relevant to your situation, so all you need to do is make the linter happy.

One approach would be to declare the variable with const instead (best to declare variables with const when possible, and let when not).

const comment_ul = document.querySelector('.comment_ul');

Another would be to use forEach instead of for(....

That said, your code looks buggy at the moment - you're adding an event listener to every button every time comment_btn is clicked. Instead of doing that, consider adding only a single listener, ever - use event delegation. If the clicked element or one of its ancestors is a .comment_a, remove its parent.

commentContainer.addEventListener('click', (event) => {
  const a = event.target.closest('.comment_a');
  if (a) {
    a.parentElement.remove();
  }
});

"*use forEach instead of for(....*" - I don't think that would help. It may shut up the linter, but it won't solve the problem. — Bergi, Nov 30 '22 at 00:25
I'm pretty confident `comment_ul` isn't being reassigned, in which case there's no problem to solve, other than shutting up the linter. — CertainPerformance, Nov 30 '22 at 00:27

score 0 · Accepted Answer · answered Nov 30 '22 at 00:26

Every time you add a new li, you are selecting EVERY delete anchor and adding another click event to it.

You should not be looping at all. You should just be selecting the anchor in the li you create and attach the event to that.

const comment_ul = document.querySelector("ul");
const comment_text = document.querySelector("#comment_text");


comment_btn.addEventListener('click', function() {
  if (!comment_text.value.length) {
    alert('Please input!');
    return;
  }

  var comment_li = document.createElement('li');

  comment_li.className = 'comment_li';

  comment_li.innerHTML = comment_text.value + "<a class='comment_a' href='javascript:;'>Delete</a>";

  comment_ul.insertBefore(comment_li, comment_ul.children[0]);

  var del = comment_li.querySelector('.comment_a');
  del.addEventListener('click', function(evt) {
    evt.preventDefault();
    comment_ul.removeChild(this.parentNode);
  });

});

<ul></ul>

<input type="text" id="comment_text" />
<button id="comment_btn">Add</button>

Functions declared within loops referencing an outer scoped variable may lead to confusing

This is a jshint warning question.How can I solve this problem?

3 Answers3