For of { onclick = () => } in Javascript: Functions declared within loops referencing an outer scoped variable may lead to confusing semantics

Question

I'm getting a JSHint warning JSHint.com

I was told my problem is: onclick = () => That is "function" in "for" (JavaScript). Some one can help me fix that?

// Sentence Bold:
const word = document.getElementById('hanzi').innerHTML;
const sentence = document.getElementById('sentence').innerHTML;
if (word) {
    const words = word.split(' ');
    let sentenceBold = sentence;
    words.forEach(item => {
        sentenceBold = sentenceBold.replaceAll(item, '<b>' + item + '</b>');
    });
    const sentenceElement = document.getElementById('sentence-bold');
    sentenceElement.innerHTML = sentenceBold;
    const bolds = sentenceElement.getElementsByTagName('b');
    for (let el of bolds) {
        el.onclick = () => {
            writeHanzi(el.innerHTML);
            updateLookupHref(el.innerHTML);
        };
    }
    writeHanzi(bolds[0].innerHTML);
    updateLookupHref(bolds[0].innerHTML);
}

What is the problem? Do you get any error? If you don't want to use an arrow function as a event handler, change it to: `el.onclick = function(event) { ... }` — Yousaf, Aug 17 '21 at 07:56
Instead of `el.onclick...` you could try `el.addEventListener("click", (event) => {... your function body})` — ckoala, Aug 17 '21 at 07:58
Also, I see that in `words.forEach(item => { sentenceBold = sentenceBold.replace(...)})` you are repeatedly assigning new values to sentenceBold. I suppose, you want to append the words. So you could use `sentenceBold +=... ` — ckoala, Aug 17 '21 at 08:01
https://i.imgur.com/KswFwvo.png This is the idea i want to implement, thank you so much for your support! — Thanh Tùng Lê, Aug 17 '21 at 08:43

score 0 · Answer 1 · answered Aug 17 '21 at 08:02

There is a lot of stuff going on with your code that is not ideal. I would almost say this particular jshint warning is the least of your problems.

When you want to set an elements content to bold conditionally you should use a css class.

You can add a css class to an element like this el.classList.add("banana");.

Also defining the onclick behaviour using el.onclick = ... is bad practice because you can only define one listener this way for each object. Use el.addEventListener("click",()=> ...) instead.

My guess is that you are just starting to code and this is fine. Code is messy in the beginning :)

score 0 · Answer 2 · answered Aug 17 '21 at 08:11

Personally I don't see much issues with the loop construction you currently have. But you can change your onclick function to make use of the event parameter to get the target that invoked the click event:

el.onclick = event => {
  writeHanzi(event.currentTarget.innerHTML);
  updateLookupHref(event.currentTarget.innerHTML);
};

Or you can change the arrow function to a regular function expression and use the this to get the clicked element.

el.onclick = function() {
  writeHanzi(this.innerHTML);
  updateLookupHref(this.innerHTML);
};

In both cases, you're not referencing el inside of your callback function anymore.

I would recommend to use .addEventListener instead of .onclick so that you always can add additional event listeners, without overwriting the previous one. The event parameter or the this remain the same regardless of the one you use.

el.addEventListener('click', function() {
  writeHanzi(this.innerHTML);
  updateLookupHref(this.innerHTML);
});

For of { onclick = () => } in Javascript: Functions declared within loops referencing an outer scoped variable may lead to confusing semantics

2 Answers2