Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Magento UI - Cleanup of undefined mixins parameters and usage of "leaking" variables scope #11371

Merged
merged 2 commits into from
Oct 25, 2017

Conversation

Igloczek
Copy link
Contributor

@Igloczek Igloczek commented Oct 11, 2017

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 magento-engcom-team changed the base branch from develop to 2.3-develop October 20, 2017 15:32
@vrann vrann self-assigned this Oct 24, 2017
@vrann vrann added this to the October 2017 milestone Oct 24, 2017
@okorshenko okorshenko merged commit 4d2c4a3 into magento:2.3-develop Oct 25, 2017
okorshenko pushed a commit that referenced this pull request Oct 25, 2017
@Igloczek Igloczek deleted the magento-ui-less-cleanup branch November 16, 2017 11:06
@Karlasa Karlasa mentioned this pull request Aug 9, 2018
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants