declare var to fix scope error

[keithbentrup]

By not declaring var i = 0, the code is referencing the i declared in the outer function. This causes an infinite loop condition that crashes browsers since multiple methods modify i from the other scope.

Description

Fixed Issues (if relevant)

1. MAGETWO-91158
2. Magento 2.2.4 - Condition Category Chooser Crashes Page if Store has Several Nested Categories #15121

[magento-cicd2]

All committers have signed the CLA.

[keithbentrup]

Just adding some more context for anyone searching for this issue ...

When adding a new marketing rule (or maybe any action that creates the catalog tree checkbox view (the category chooser) in the admin), the browser (tested on FF & Chrome) will hang on catalogs with child categories where buildCategoryTree and processCategoryTree are called infinitely as both will ultimately modify the i counter var outside their own scope, so the end condition is never reached.

[hostep]

@keithbentrup: your extra description reminded me of this very recent ticket: #15121
It looks like applying your fixes also fixes that ticket. Might be best to add it to the Fixed Issues (if relevant) list.

Nice work btw!

[keithbentrup]

@hostep Yep, that issues describes the scenario perfectly. Added to the fixed issues.

[stkristobal]

Lifesaver @keithbentrup !

[VladimirZaets]

Hi @kieronthomas, thank you for collaboration.
According to Magento Code Style guideline we should declare a variables in the top of the function.

[kieronthomas]

@VladimirZaets nothing to do with me, I think you meant to refer to @keithbentrup ^

[keithbentrup]

Ok @VladimirZaets so update it accordingly if you need to. It's a P1 ticket that's crashing browsers. Fix it and move on.

Incidentally, that rule may be part of the reason why the error exists in the first place ... with this perhaps ill-advised rule, declaring vars far removed from their use.

For your consideration,

1. https://stackoverflow.com/questions/5053073/is-defining-every-variable-at-the-top-always-the-best-approach
2. https://medium.com/@bluepnume/theres-no-need-to-define-all-javascript-vars-once-at-the-top-of-a-function-and-there-hasn-t-been-a66b31f21822

Also linking to the whole style guide without linking to the specific rule isn't particularly helpful. You're asking contributors to follow rules that you can't even find/link to.

I couldn't even find the rule googling and limiting the domain to just site:devdocs.magento.com. The most relevant section that I could find with a bit of searching which has no mention of declaring it at the top: https://devdocs.magento.com/guides/v2.3/coding-standards/code-standard-javascript.html#variable-declarations. You have to go parse the eslint configuration json to actually find the rule you're bringing up.

Lastly, I doubt you're going to fix all the other unrelated errors picked up by the linter for this file, so arbitrarily fixing it for these and not others seems capricious.

Anyway, I'm more concerned with this being resolved. You now know the cause of the error and the solution. You don't need anything further from me to bring it to a resolution.

[orlangur]

@keithbentrup if there is such rule we cannot violate it. Changing existing conventions is out of scope for PR processing.

@VladimirZaets we can probably move var declaration to a proper place on our side as an additional commit.

[magento-engcom-team]

@keithbentrup thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[DanielRuf]

These are loop variables and they should be set with var so this is right imho.

[VladimirZaets]

@DanielRuf In JS loops are haven't the self scope. The var declaration should be, you are right, but according to Magento Code Styles and ESLint rules, declaration variable should be on the top of the function.

[VladimirZaets]

@orlangur yes, I will fix it.

[DanielRuf]

declaration variable should be on the top of the function.

Oh well, this makes it harder to keep track of them as loops like in the changeset should use their own local variable instances instead of using a variable which is available in the context of the whole function. Also then you have to rename one of them and create unique variable names.

Personally I find this Code Styles rule for loops wrong and is not best practice in the frontend / JS world.

[VladimirZaets]

@DanielRuf
In JS (ES5) loops can't have local variables, in JS the context of a variable is limited to a function.
All variables that are declared inside the function automatically hosted to the top of the function with undefined value.

For it, we add the rule for ESLint to do this process more transparently.

You can read more about the hosting and variable scopes in JS.

[DanielRuf]

In JS (ES5) loops can't have local variables

I see it differently. Loops should always have the var declaration in the condition / head.

All variables that are declared inside the function automatically hosted to the top of the function with undefined value.

Not what is in the head of the loop and not if you use let.

I guess you mean hoisting and I know what this is ;-)

in JS the context of a variable is limited to a function

No, a block is a context. A block is also a loop (anything with a curly brace in most cases).

In general let should be used here to prevent issues and this is the right approach.

https://stackoverflow.com/a/36838559/753676

https://caniuse.com/#feat=let

[DanielRuf]

I would propose to use let instead of var if you see any issue with hoisting, memory leakage and undefined variables.

[DanielRuf]

Also see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for for best practice (var used in head of loop).

[orlangur]

@DanielRuf let looks like a good option to me (then we should add such recommendation for loops to best practices guide also).

For the Mozilla example, it does not really matter if other projects have other rules, Magento coding style can always be reasonably changed (hope so, never saw in action :)), but it should be some separate effort, not in scope of an arbitrary PR.

[VladimirZaets]

@DanielRuf
Yes, I agree, use let in the current case is the best approach. But we support old browsers, that haven't fully support ES6.

We haven't any transpalier like Babel, so all our JS code is written according to ES5 standard.

I don't sure that examples https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for is the best practice rather than the simple overview of for loop.

From the book JavaScript: The Good Parts Douglas Crockford:

JavaScript has C syntax, but its blocks don’t have scope. So, the convention that variables should be declared at their first use is really bad advice in JavaScript. JavaScript has function scope, but not block scope, so I declare all of my variables at the beginning of each function. JavaScript allows variables to be declared after they are used. That feels like a mistake to me, and I don’t want to write programs that look like mistakes.

[DanielRuf]

but its blocks don’t have scope
but not block scope

That's not right anymore with let, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/block =) Douglas is known for his opinions and views which are contrary to what is happening in the TC39.

But we support old browsers

https://devdocs.magento.com/guides/v2.0/install-gde/system-requirements_browsers.html

Officially IE11+ so you can still use let or which exact and old browsers do you meant that are officially supported in Magento 2?

[orlangur]

@VladimirZaets are you referring to a partial support in IE11 https://caniuse.com/#feat=let ? I'm not sure if it can cause any troubles, but maybe there are some browsers not mentioned in this table but requiring support.

[DanielRuf]

I'm not sure if it can cause any troubles

Normally not as we use let and const in projects.
iOS looks a bit different regarding support but Safari is not tied to the iOS version afaik. But all but iOS 10 and 11 get support by Apple.

Well, then let's create unique var names. The reuse of such a iterator variable / pointer and defining this on top of the function / block scope does not feeld good generally and can introduce other issues.

[jonshipman]

See also line 188 for similar issue.

[jonshipman]

Try this:

        categoryLoader.buildCategoryTree = function (parent, config) {// eslint-disable-line no-shadow
            var i = 0;

            if (!config) {
                return null;
            }

            if (parent && config && config.length) {
                while (i < config.length) {
                    categoryLoader.processCategoryTree(parent, config, i);
                    i++;
                }
            }
        };

        categoryLoader.buildHashChildren = function (hash, node) {// eslint-disable-line no-shadow
            var i = 0, len;

            // eslint-disable-next-line no-extra-parens
            if ((node.childNodes.length > 0) || (node.loaded === false && node.loading === false)) {
                len = node.childNodes.length;
                hash.children = [];

                while (i < len) {
                    /* eslint-disable */
                    if (!hash.children) {
                        hash.children = [];
                    }
                    /* eslint-enable */
                    hash.children.push(this.buildHash(node.childNodes[i]));
                    i++;
                }
            }

            return hash;
        };

[DanielRuf]

See also line 188 for similar issue.

"Same" issue. There are just these 2 changes here.

Interesting, still old Varien code =)

I guess it should be ok to set the vars at the top of these (very little) methods but it feels wrong for me as frontend dev working with Node.js and JavaScript on a daily basis. ;-)

[jonshipman]

Sorry Dan, came to this pull from #15121. And was pointing out the issue with scope in the next function as well.

[magento-engcom-team]

Hi @keithbentrup. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.2.5 release.

Please, consider porting this solution across release lines.
You may use Porting tool to port commits automatically.