How to add eventListeners within a loop in JavaScript without triggering JSHint's confusing semantics error?

Viewed 52

I am working on creating a dynamic list of items in JavaScipt using DOM. Each item has a link (an HTML element) that links to another element on the page; a form that allows the user to edit the team. The link works fine, but I'm attempting to make the form work by adding an eventListener to the link, since the form needs to get the team that we are editing as a reference.

Here's the code in question:

    for (let i = 0; i < teams.length; i++) {
    let row = table.insertRow(-1);
    //removed code that made cells under the row
    let link = document.createElement("a");
    link.setAttribute("href","#form");
    link.team = teams[i];
    link.addEventListener("click", function (e) {
        updateForm(link.team);
    });
    link.textContent = teams[i].name;
    //code that adds the link and other elements into the cells of the table
}

I have removed some parts of the code for clarity. The important thing is the addEventListener, which functions perfectly. The way it is right now currently correctly sends the team in question when the user clicks on the button. However, I am getting the following message from JSHint:

(error) Functions declared within loops referencing an outer scoped variable may lead to confusing semantics.

If I've understood correctly, this is because the variable tableexists outside of the scope of the loop. However, I do not understand why getting the table inside of the loop would change anything. What exactly am I supposed to change in the code to make sure that JSHint would be happy? I can also use some other method than adding an eventListener if it seems like a better solution.

Cheers!

2 Answers

If I've understood correctly, this is because the variable tableexists outside of the scope of the loop.

link, not table.

What exactly am I supposed to change in the code to make sure that JSHint would be happy?

I'd come at it from the other side: What do you need to change in JSHint / your linting setup so that your perfectly-valid code using link in a callback doesn't trigger a linting error? If your code were using var the warning would be very useful, but your code is using let.

IIRC, JSHint is much more configurable than the JSLint it was originally based on (years ago now), you might look to see if there's an option that turns off this particular warning when you're using let for link.

Alternatively, you might look into another linting solution (ESLint is quite popular).

But there is the small chance that you might change the team property of the object that link refers to. If you want to eliminate that possibility and get rid of the explicit function, you could use bind:

link.addEventListener("click", updateForm.bind(null, link.team));

That will also get rid of the JSHint warning.

Or since you're using an expando property on the element itself, you could use this:

link.addEventListener("click", function() {
    updateForm(this.team);
});

...but I'd suggest you not use expando properties on DOM elements. For instance:

for (const team of teams) {
    const row = table.insertRow(-1);
    //removed code that made cells under the row
    const link = document.createElement("a");
    link.setAttribute("href","#form");
    link.addEventListener("click", () => { // Or:
        updateForm(team);                  //     link.addEventListener("click", updateForm.bind(null, team));
    });                                    // 
    link.textContent = team.name;
    //code that adds the link and other elements into the cells of the table
}

Linters used to complain any time you defined a function in a loop b/c you weren't being DRY. (I'm kinda surprised that JSHint doesn't, but JSLint doesn't appear to think this is an error any more either!)

Like older versions of linters, I'd argue that [re]defining the same function within each iteration of your loop is a bad practice any time. Because the function declaration happens within a loop, each separate function's logic is identical to the others'. That means the code can't be DRY. It also allows complex scoping setups like yours!

link.addEventListener("click", function (e) {
    updateForm(link.team);
});    

Normally you should pull that function out of the loop and define a single reference to it outside of the loop.

BUT... You're passing variables into your function through the backdoor with some, um, creative closure use, which is a more nuanced error [that linters apparently still care about].

Here's one way you could fix the lint that makes the importance of scoping in your initial code explicit (though keep reading!).

/*jshint esversion: 6 */
/*global updateForm, teams, table */

// Note that this function returns a function containing a closure
// that maintains one `link` per call.
function myListenerFactory(link) {
    return function (e) {
        updateForm(link.team);
    };
}

for (let i = 0; i < teams.length; i++) {
    let row = table.insertRow(-1);
  
    //removed code that made cells under the row
  
    let link = document.createElement("a");
    link.setAttribute("href","#form");
    link.team = teams[i];
    
    // Use the same function each time. Note that it's called IMMEDIATELY,
    // returning a new function for each iteration.
    link.addEventListener("click", myListenerFactory(link)()); // <<< Same function reused in the loop
    
    link.textContent = teams[i].name;
    
    //code that adds the link and other elements into the cells of the table
}

That lints on jshint.com.

This code maintains the closure from the original by using a factory and returns one scope-wrapped function per loop iteration.

It's nice in that it clearly shows the protected scope is intentional -- you wanted a new function per iteration because scope was important -- and also limits what's in that protected scope to link and nothing else, but otherwise has no benefit.

This is why TJ says "your perfectly-valid code using link in a callback" -- your code depends on the closure, so each redefinition is, technically, different, though only by its scope. It is DRY from the start. It just doesn't look like it. It doesn't look like what it does, imo.

(That we're counting on variable hoisting to pass link to our function should be all the code smell you need to know there's probably a better solution.)


A better solution

So I'd want to push on "perfectly-valid" a bit, and I think TJ does too, by saying to avoid expando properties. It's not the easiest code to grok (nor is the lint-passing alternative, above), and that's what the linter has identified.

Let's try to find a better option.

I'm pretty sure you can get the reference you want from the e.target (unless you're supporting IE 8 or lower). Then we don't have to pass around the link at all. We can derive it from our event.

/*jshint esversion: 6 */
/*global updateForm, teams, table */

function myEventHandler(e) {
    // No need to backdoor `link` when we can derive it from `e`!
    updateForm(e.target.team);
}

for (let i = 0; i < teams.length; i++) {
    let row = table.insertRow(-1);
  
    //removed code that made cells under the row
  
    let link = document.createElement("a");
    link.setAttribute("href","#form");
    link.team = teams[i];
    
    // Use the same function each time
    link.addEventListener("click", myEventHandler);
    
    link.textContent = teams[i].name;
    
    //code that adds the link and other elements into the cells of the table
}

And boom. No wacky scope, no repeated function declaration. Perfect.


Expando warning

Again, TJ's warning about avoiding "expando properties" is a very good one. Expando properties are nonstandard ones you smash onto a DOM element after the object is initialized.

These properties can be edited and/or removed without warning by other libraries fairly commonly. You'd probably be better off using data attributes...

... or, my preference, having a lookup table that's accessed by adding unique element ids to each of your links.

That is, I'm not in love with TJ's solution of keeping your initial scoping-dependent hipstery ;^D, but it works too.

Make sense?

Related