[Backport] Magento UI - Cleanup of undefined mixins parameters and usage of "leaking" variables scope

[mageprince]

Original Pull Request

#11371

Backstory

It's not a secret that I don't like LESS, but because of SASS Blank project, I often have to deep dive into Magento UI LESS library code.
Unfortunately, 9 of 10 times I got a punch of weirdness of language "features" mixed with the inconsistency of the implementation right in the face.
That's why I decided to do something with it and clean up most annoying places.

It may be considered as heavily opinionated changes, but all of this code is 100% safe and didn't change the output CSS, the goal is to make working on this code less painful, especially for porting into languages that care more about scope and execution order, but it also should help people working directly on LESS, because all of the changes can be considered as a good practice in any programing language.

Technical reason of changes

In LESS variables have kinda weird scope rules, which I'm considering as a leaking scope and the source of confusion i.e. functions (mixins) invoked inside function declaration inherits all variables of the parent function.

An example in a JS-like code to show weirdness of the LESS scoping:

function parent (param) {
  child();
}

function child() {
  return param;
}

parent('red'); // it will return 'red', but should throw an error of undefined variable

It's not a common practice in other languages used in FE development and it not clear for developers, so will be nice to avoid it.

The goal is to pass all necessary values to mixin only through parameters or using global variables as default values of parameters.

Same example, but with this new rules applied:

const sample = 'red';

function parent (param = sample) {
  child(param);
}

function child(param) {
  return param;
}

parent(); // it will return 'red'
parent('blue'); // it will return 'blue'

Description of changes

I removed all fallbacks to variables not existing in the global scope, defined all variables used inside mixins as parameters and added all missing parameters to the places where mixins are invoked.
Also mixin that was used just once, was moved and simplified, to reduce usage of weird LESS scoping, where variables defined in mixin are accessible in the place where mixin is invoked.

[magento-engcom-team]

Hi @mageprince. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me {$VERSION} instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[magento-engcom-team]

Hi @mageprince. Thank you for your contribution.
We will aim to release these changes as part of 2.2.6.
Please check the release notes for final confirmation.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.

[TomsBugna]

@mageprince @Igloczek You should not just simply like that remove mixin methods from code while those methods are still called in other files and are not removed as well. It is regarding this commit.

I updated my Magento2 to version 2.2.6 and grunt started to throw errors and could not finish the process. Here is the aborting error messages:

Running "less:documentation" (less) task
NameError: .lib-url-check is undefined in lib/web/css/docs/source/_utilities.less on line 381, column 5:
380 // "Call" the mixin
381 .lib-url-check(@_icon-image);
382
Warning: Error compiling lib/web/css/docs/source/docs.less Use --force to continue.

Aborted due to warnings.

#17097

[Igloczek]

@tomijs this documentation is unmaintained, that's why was ignored in these changes.

[TomsBugna]

@Igloczek but still this commit was included in 2.2.6 version and the documentation with old calls are still there. And less:documentation is also called in Gruntfile.js. Nevertheless, with this I wanted to say that errors appear while doing grunt in the latest magento2 public version (2.2.6).

[Igloczek]

CC @DrewML you might want to know about this problems. For me it should be considered as legacy and removed. If that's not possible, probably need some serious updates and some test coverage on CI, to be sure it at least cam be built.