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

each() loop w/ & behaves funny #3340

Closed
calvinjuarez opened this issue Dec 5, 2018 · 10 comments
Closed

each() loop w/ & behaves funny #3340

calvinjuarez opened this issue Dec 5, 2018 · 10 comments

Comments

@calvinjuarez
Copy link
Member

Given the following, I would expect both loops to render similar output. However, the version where the each() is inside the block and & is used produces strange results. Specifically, the styles immediately under the & are rendered at the top of the loop's generated output. Oddly, a child of the & block is rendered in the expected spot.

Given

@infixes: a, b;

/*! LOOP IN BLOCK */
.amp {
  each(@infixes, #(@_infix, @i) {
    // Each infix actually generates the _next_ infix's block, with the last
    // one generating styles with no infix.
    @infix: e(if(
      ((@i + 1) <= length(@infixes)),
        %('-%s', extract(@infixes, (@i + 1))),
        ''
    ));

    &@{infix} {
      display: block;
      .child {
        display: block;
      }
    }
  });
}

/*! BLOCK IN LOOP */
each(@infixes, #(@_infix, @i) {
    // Each infix actually generates the _next_ infix's block, with the last
    // one generating styles with no infix.
  @infix: e(if(
    ((@i + 1) <= length(@infixes)),
      %('-%s', extract(@infixes, (@i + 1))),
      ''
  ));

  .plain@{infix} {
    display: block;
    .child {
      display: block;
    }
  }
});

Expected

/*! LOOP IN BLOCK */
.amp-b {
  display: block;
}
.amp-b .child {
  display: block;
}
.amp {
  display: block;
}
.amp .child {
  display: block;
}
/*! BLOCK IN LOOP */
.plain-b {
  display: block;
}
.plain-b .child {
  display: block;
}
.plain {
  display: block;
}
.plain .child {
  display: block;
}

Actual

/*! LOOP IN BLOCK */
.amp {
  display: block;
}
.amp-b {
  display: block;
}
.amp-b .child {
  display: block;
}
.amp .child {
  display: block;
}
/*! BLOCK IN LOOP */
.plain-b {
  display: block;
}
.plain-b .child {
  display: block;
}
.plain {
  display: block;
}
.plain .child {
  display: block;
}

Note that .amp is floated to the top.

@calvinjuarez
Copy link
Member Author

Detached rulesets also behave unexpectedly.

@infixes: a, b;

/*! DETACHED RULESET */
.dr {
  each(@infixes, #(@_infix, @i) {
    @infix: e(if(
      ((@i + 1) <= length(@infixes)),
        %('-%s', extract(@infixes, (@i + 1))),
        ''
    ));
    
    @content();
  });
}

@content: {
  &@{infix} {
    display: block;
    .child { display: block; }
  }
};
/*! DETACHED RULESET */
.dr {
  display: block;
}
.dr-b {
  display: block;
}
.dr-b .child {
  display: block;
}
.dr .child {
  display: block;
}

@matthew-dean
Copy link
Member

Thanks @calvinjuarez. Do you think you could boil that down to the simplest example that demonstrates the bug? Or are %(), extract(), e() and length() essential to trigger the issue? Right now the example is hard to follow, as to what is doing what.

@calvinjuarez
Copy link
Member Author

@matthew-dean all those functions are just to say, "whatever the current item is, use the next one, unless this is the last one, in which case, use ''". It's important to loop that way to see the bug. Otherwise, the bug is invisible because the un-infixed version is supposed to be at the top. Does that make sense?

So in the loop, when a is the current value, it uses the next value, -b as the actual infix. When b is the current value, since it's the last value, the infix is an empty string (escaped).

So, to answer the question, no, they're not essential to trigger it (I expect it happens always), they're just a way of looping in the wonky way that exposes the issue.

I ran into the issue while fixing the output order in seanCodes/bootstrap-less-port, where the same behavior is accomplished with breakpoint-next() and breakpoint-infix() functions, but the result is the same.

I'll see if there's a less obtuse test case that produces a similar error.

@calvinjuarez
Copy link
Member Author

calvinjuarez commented Dec 6, 2018

Here's something a tiny bit more concise, and hopefully easier to read. The key is that, in the CSS, when @infix is an empty value, that output is above output when @infix is a non-empty value, even when the empty value should come afterward. This doesn't happen when the class is inside the loop.

It looks like it has something to do with how & works when the selector ends up matching its parent.

@infixes: a, b;

/*! LOOP IN BLOCK */
.amp {
  each(@infixes, #(@val, @i) {
    @is-last: boolean((@i = length(@infixes)));
    @infix: if(@is-last, ~'', ~'-@{val}');

    &@{infix} { -less-original-value: @val; }
  });
}

/*! LOOP IN BLOCK – CONTROL */
.control {
  each(@infixes, #(@val, @i) {
    @is-last: boolean((@i = length(@infixes)));
    @infix: if(@is-last, ~'-', ~'-@{val}');
    // ^ identical to previous except @infix is non-empty, even when @is-last is true

    &@{infix} { -less-original-value: @val; }
  });
}
/*! LOOP IN BLOCK */
.amp {
  -less-original-value: b;
}
.amp-a {
  -less-original-value: a;
}
/*! LOOP IN BLOCK – CONTROL */
.control-a {
  -less-original-value: a;
}
.control- {
  -less-original-value: b;
}

@calvinjuarez
Copy link
Member Author

calvinjuarez commented Dec 6, 2018

I'm getting the feeling this is expected behavior out of &, actually. Consider:

.control-2 {
  each(@infixes, #(@val, @i) {
    @infix: ~''; // @infix is always empty

    &@{infix} { -less-original-value: @val; }
  });
}
.control-2 {
  -less-original-value: a;
  -less-original-value: b;
}

That's probably expected. This looks like it'd be a hairy one to sort out...unless we're okay making this output as 2 rules with the same selector. :/

@seven-phases-max
Copy link
Member

seven-phases-max commented Dec 6, 2018

At quick glance I suppose .amp is floated to the top because it is at the top. E.g. consider:

.amp {
    /* 0 */
    a {/* 1 */}
    & {/* 2 */}
    b {/* 3 */}
}

this should result in:

.amp {
    /* 0 */
    /* 2 */
}

.amp a {/* 1 */}
.amp b {/* 3 */}

And the same order is expected if you remove /* 0 */.
I.e. it's two instances of .amp that simply collapse into the first of them (the surrounding "block" itself) which is always there and always at the top even if empty (like in the initial example).

@matthew-dean
Copy link
Member

matthew-dean commented Dec 6, 2018

Yeah, I never really thought about placement order of nested blocks. But it definitely seems like Less doesn't try to keep things in order. To expand on @seven-phases-max's example:

.amp {
  is: one;
  .a { is: two }
  & { is: three }
  .b { is: four }
  is: five;
}

Outputs:

.amp {
  is: one;
  is: three;
  is: five;
}
.amp .a {
  is: two;
}
.amp .b {
  is: four;
}

Sass's output is slightly different, opting to keep nested selectors in order, even if one is just &

.amp {
  is: one;
  is: five;
}
.amp .a {
  is: two;
}
.amp {
  is: three;
}
.amp .b {
  is: four;
}

Oddly, Sass doesn't treat the is: five declaration as coming "after" the other nested declarations. But I guess that does allow you to "force" subsequent declarations to come in a certain order by wrapping them in &{}.

So, if you're migrating Bootstrap and comparing output, you're likely seeing that Less collapses & as being part of the parent ruleset, and Sass treats it as a new selector and generates a sequence of rules to follow the parent.

The each() function doesn't do anything special in terms of rulesets. It basically generates a sequence of anonymous rulesets (injecting @value, @key etc variables at the top of each one) which then get evaluated.

As far as which behavior makes more sense? 🤷‍♂️ I really don't know how to answer that. They're just different. I think the rationale in Less's behavior is to create fewer selectors. But Sass's rationale is probably to keep nested selector order intact, but it probably comes down to the merging algorithms are just different.

Now, I were to put my CSS hat on, and thinking about things like the @nest proposal, probably BOTH Less and Sass have this wrong (with Sass maybe being "more" wrong, because it places three as the "final" value in the .amp class?). What the output probably SHOULD be to reflect the way it was written is this:

.amp {
  is: one;
}
.amp .a {
  is: two;
}
.amp {
  is: three;
}
.amp .b {
  is: four;
}
.amp {
  is: five;
}

That is, declarations should maintain their order in the stylesheet, if they're broken up by other nested selectors. If nesting were ever added to CSS natively, that's likely what it would mean. It's more verbose, but it seems more accurate.

@calvinjuarez
Copy link
Member Author

calvinjuarez commented Dec 6, 2018

Yeah, I think we're all seeing eye-to-eye. What we've got is reliable and expected in nearly all cases (the examples shared in the previous 2 comments, for instance). You kind of have to work to find how to "break" it. Even in those situations, though, the workaround doesn't suck, since each() can run at the root level without anything in the way, so it's probably usually gonna be easy to write out the need for the "naked" & entirely.

Given all that, I'm closing this for now (won't fix, intended behavior). If we really wanted to allow forcing output the way sass does, it would need to be a new feature, and it'd need a SUPER compelling use case.

And for posterity:

Workaround

Refactor so that & doesn't appear alone (even conditionally) at the root of the loop's iterator mixin.

.amp {
  each(a b, #(@val, @i) {
    @is-last: boolean((@i = length(@infixes)));
    @infix: if(@is-last, ~'', ~'-@{val}');

    &@{infix} {
      -less-original-value: @val;
    }
  });
}

BECOMES

each(a b, #(@val, @i) {
  @is-last: boolean((@i = length(@infixes)));
  @infix: if(@is-last, ~'', ~'-@{val}');

  .amp@{infix} {
    -less-original-value: @val;
  }
});

calvinjuarez added a commit to calvinjuarez/bootstrap-less-port that referenced this issue Dec 6, 2018
calvinjuarez added a commit to calvinjuarez/bootstrap-less-port that referenced this issue Dec 6, 2018
calvinjuarez added a commit to calvinjuarez/bootstrap-less-port that referenced this issue Dec 6, 2018
calvinjuarez added a commit to calvinjuarez/bootstrap-less-port that referenced this issue Dec 6, 2018
@matthew-dean
Copy link
Member

@calvinjuarez Btw, you can simplify your code slightly like this:

@infixes: a b;
each(@infixes, #(@val, @i) {
  @is-last: boolean(@i = length(@infixes));
  @infix: if(not(@is-last), ~'-@{val}');

  .amp@{infix} {
    -less-original-value: @val;
  }
});

That is, boolean and if no longer require outer parens around first argument (as of 3.6...?), and an if() statement doesn't necessarily need an else condition.

calvinjuarez added a commit to calvinjuarez/bootstrap-less-port that referenced this issue Dec 7, 2018
calvinjuarez added a commit to calvinjuarez/bootstrap-less-port that referenced this issue Dec 7, 2018
calvinjuarez added a commit to calvinjuarez/bootstrap-less-port that referenced this issue Dec 7, 2018
calvinjuarez added a commit to calvinjuarez/bootstrap-less-port that referenced this issue Dec 7, 2018
@calvinjuarez
Copy link
Member Author

Interesting. I tried dropping the parens, but it didn't compile for me at the time. I'll double-check. The bootstrap-less-port handles that logic with plugin functions (to be more familiar to folks coming from the Sass stuff), but I'll have to keep that in mind in case we decide to go native-er with it at some point.

calvinjuarez added a commit to seanCodes/bootstrap-less-port that referenced this issue Feb 26, 2019
* package – update css-compile script to use --math

--strict-math has been deprecated and replaced by --math.  See less/less.js#3274

* maps – begin converting to DRs

* each – use each() for loops (WIP)

Still haven't updated for the grid breakpoints just yet, though some breakpoint loops have been written in anticipation of those updates.

* breakpoints – fix breakpoint JS after converting to DR maps

* each – replace all #each-* loop with each or #for-*

There are 4 loops in mixins/_grid-framework.less that loop 'til a static number.  I'm not aware of any answer for that at the moment, but a plugin could be easily written that could just leverage `each()`.

* dev, meta – steal nice stuff from #12

Thanks @matthew-dean!

* plugins – fix lint warnings/errors

Except one in breakpoints.js:

```
91:6   warning  Variable 'nextBreakpoint' should be initialized on declaration
```

* _tables – fix .table-responsive output order

See less/less.js#3340

* _navbar – fix .navbar-expand output order

See less/less.js#3340

* README – update to match true min version required

Co-Authored-By: calvinjuarez <[email protected]>

* plugins/theme-color-level – more readable code

Co-Authored-By: calvinjuarez <[email protected]>

* README – replace the other, less easy ways of installing

* _grid-framework – restore comment

See #14 (comment)

* README – wording edits

Co-Authored-By: calvinjuarez <[email protected]>

* _functions – restore commented Sass

* README – more wording edits

Co-Authored-By: calvinjuarez <[email protected]>

* README – remove clone note

per #14 (comment)

* plugins/breakpoints – remove parseUnit(); fix nextBreakpoint ESLint issue

See https://github.com/seanCodes/bootstrap-less-port/pull/14/files/94a03d156dde954caccf58130ebc8fa3adaf397e#r240023403

* rename plugin `keys` → `map-keys`

Reverting the name change for now, until @calvinjuarez is able to pull
in the plugin from NPM and alias it. (See [his
comment](#14 (comment)
sion_r241965904).)

This change addresses [this PR review
comment](#14 (comment)
sion_r239916331).

* _grid-framework – revert loop renaming

Reduce the number of changes in the PR to avoid potential bugs.

This change addresses [this PR review comment](#14 (comment)).

* _transition – revert var renaming

Match var names used in the Sass version of the mixin.

This change addresses [this PR review comment](#14 (comment)).

* breakpoints – put long comments on own line

(instead of including at the end of a line)

* restore color-function plugins

Revert deletion of plugin files that add the `color()`, `gray()` and
`theme-color()` functions. Even though Less’ new ruleset accessors could
be used, we’re going to keep them for parity with the Sass version and
for backwards compatibility.

This change addresses [this PR review comment](#14).

* restore usages of color functions

Revert the conversion the `color()`, `gray()` and `theme-color()`
functions to Less’ ruleset accessors. This change is primarily being
made to keep the syntax as similar to the Sass version as possible, for
easy reference/comparison.

Note that keeping things similar to Sass may prove unnecessary in the
future, so we may go back to accessors, but for now we’ll keep it like
this.

This change addresses [this PR review comment](#14).

* change color functions to handle rulesets vs lists

* fix bug in ruleset-to-map conversion

Instead of pulling the `value` directly from each item in the ruleset,
evaluate the item first to determine the true value of the item. This
fixes the issue where the `value` is actually a var name and what we
want is the the value of the var—not the var name itself.
seanCodes pushed a commit to seanCodes/bootstrap-less-port that referenced this issue Mar 3, 2019
* package – update css-compile script to use --math

--strict-math has been deprecated and replaced by --math.  See less/less.js#3274

* maps – begin converting to DRs

* each – use each() for loops (WIP)

Still haven't updated for the grid breakpoints just yet, though some breakpoint loops have been written in anticipation of those updates.

* breakpoints – fix breakpoint JS after converting to DR maps

* each – replace all #each-* loop with each or #for-*

There are 4 loops in mixins/_grid-framework.less that loop 'til a static number.  I'm not aware of any answer for that at the moment, but a plugin could be easily written that could just leverage `each()`.

* dev, meta – steal nice stuff from #12

Thanks @matthew-dean!

* plugins – fix lint warnings/errors

Except one in breakpoints.js:

```
91:6   warning  Variable 'nextBreakpoint' should be initialized on declaration
```

* _tables – fix .table-responsive output order

See less/less.js#3340

* _navbar – fix .navbar-expand output order

See less/less.js#3340

* README – update to match true min version required

Co-Authored-By: calvinjuarez <[email protected]>

* plugins/theme-color-level – more readable code

Co-Authored-By: calvinjuarez <[email protected]>

* README – replace the other, less easy ways of installing

* _grid-framework – restore comment

See #14 (comment)

* README – wording edits

Co-Authored-By: calvinjuarez <[email protected]>

* _functions – restore commented Sass

* README – more wording edits

Co-Authored-By: calvinjuarez <[email protected]>

* README – remove clone note

per #14 (comment)

* plugins/breakpoints – remove parseUnit(); fix nextBreakpoint ESLint issue

See https://github.com/seanCodes/bootstrap-less-port/pull/14/files/94a03d156dde954caccf58130ebc8fa3adaf397e#r240023403

* rename plugin `keys` → `map-keys`

Reverting the name change for now, until @calvinjuarez is able to pull
in the plugin from NPM and alias it. (See [his
comment](#14 (comment)
sion_r241965904).)

This change addresses [this PR review
comment](#14 (comment)
sion_r239916331).

* _grid-framework – revert loop renaming

Reduce the number of changes in the PR to avoid potential bugs.

This change addresses [this PR review comment](#14 (comment)).

* _transition – revert var renaming

Match var names used in the Sass version of the mixin.

This change addresses [this PR review comment](#14 (comment)).

* breakpoints – put long comments on own line

(instead of including at the end of a line)

* restore color-function plugins

Revert deletion of plugin files that add the `color()`, `gray()` and
`theme-color()` functions. Even though Less’ new ruleset accessors could
be used, we’re going to keep them for parity with the Sass version and
for backwards compatibility.

This change addresses [this PR review comment](#14).

* restore usages of color functions

Revert the conversion the `color()`, `gray()` and `theme-color()`
functions to Less’ ruleset accessors. This change is primarily being
made to keep the syntax as similar to the Sass version as possible, for
easy reference/comparison.

Note that keeping things similar to Sass may prove unnecessary in the
future, so we may go back to accessors, but for now we’ll keep it like
this.

This change addresses [this PR review comment](#14).

* change color functions to handle rulesets vs lists

* fix bug in ruleset-to-map conversion

Instead of pulling the `value` directly from each item in the ruleset,
evaluate the item first to determine the true value of the item. This
fixes the issue where the `value` is actually a var name and what we
want is the the value of the var—not the var name itself.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants