Stanford-Online / edx-platform

The Open edX platform, the software that powers Lagunita!
https://lagunita.stanford.edu
GNU Affero General Public License v3.0
42 stars 18 forks source link

Fix eslint errors in account settings js #845

Closed kluo closed 5 years ago

kluo commented 5 years ago

@stvstnfrd ended up just hoisting the vars since it might make it easier in future merges to see if they happens to use the same variable name or something

stvstnfrd commented 5 years ago

might make it easier in future merges to see if they happens to use the same variable name

Laudable goal, but pragmatically it's unlikely to work out. Even if they create a variable w/ the same name, we'll still get conflicts because the lines won't be identical (because we have other variables on the same line).

And if the goal is detecting duplicate variable names, there's already an ESLint check for that, no-redeclare :)

An alternate approach to keep our code together and not disable the eslint issues would be to wrap our code in an IIFE. That gives it its own function scope so the variable declarations would still be in the top of the function :)

(function () {
    var fullnameIndex, emailIndex, passwordIndex, basicFields;
    if (isShibAuth) {
        // ...
    }
})();

Realistically though, I think the simplest approach is probably to just disable/reënable the eslint errors.

/* eslint-disable vars-on-top */
if (isShibAuth) {
    // ...
}
/* eslint-enable vars-on-top */

But that said, I think the IIFE approach is probably the "best" technical approach, because we can still access the outside scope, while fully isolating our internal variables in their own closure. Or is that too "clever"?