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

Unexpected :extend() behavior (with selector { selector {} } nesting) #1487

Open
calvinjuarez opened this issue Aug 13, 2013 · 9 comments
Open

Comments

@calvinjuarez
Copy link
Member

My Input

So this:

.class, .other { /* stuff */ }
.class {
    .class, .other { /* more stuff */ }
}
.extension { &:extend(.class all); }

Current (1.4.1) Output

Currently compiles to this:

.class,
.other,
.extension {
    /* stuff */
}
.class .class,
.class .other,
.extension .extension,
.extension .other {
    /* more stuff */
}

What I'd Expect

But it seems like the result should be this:

.class,
.other,
.extension {
    /* stuff */
}
.class .class,
.class .other,
.class .extension, /* this */
.extension .class, /* this */
.extension .other,
.extension .extension {
    /* more stuff */
}

In other words, (almost) exactly like this compiles:

.class, .extension, .other { /* stuff */ }
.class, .extension {
    .class, .other, .extension { /* more stuff */ }
    /* OR */
    & &, .other { /* more stuff */ }
}

Questions

So,

  • Is there a reason for the current behavior vs. the other?
  • What's the recommended way to get the target output? (for my best effort, see "Workaround" below)
@calvinjuarez
Copy link
Member Author

Workaround

The best workaround I've found is to force the issue, although this results in some ugly selectors.

.class, .other { /* stuff */ }
.class, .extension {
    .class, .other, .extension { /* more stuff */ }
}
.extension { &:extend(.class all); }
.class,
.other,
.extension {
    /* stuff */
}
.class .class,
.extension .class,
.class .other,
.extension .other,
.class .extension,
.extension .extension, /* ugly */
.extension .extension, /* ugly */
.extension .extension, /* ugly */
.extension .other,
.extension .extension {
    /* more stuff */
}

@lukeapage
Copy link
Member

Its a bug

@calvinjuarez
Copy link
Member Author

Cool. Any plans to squash? If not, I could work on it—though I may need someone to point me in the right direction.

@lukeapage
Copy link
Member

@calvinjuarez sorry it took a while to reply - plans to squash it eventually, but we would love any help.

I would

  • checkout the 1.5.0-wip branch
  • create a test case for it by editing test/less/extends.less and test/less/extends.css
  • run make test to run the tests and verify it fails - note: I've added one extends failing tests case already and it may be the same bug
  • install node-inspector and run it
  • run node --debug-brk test/less-test.js extends
  • go to the web address that node inspector is running in
  • debug lib/less/extend-visitor.js - this file visits the AST and converts extends into selectors

please let me know if you need any more help. my email address is my username, with dots around the a on the gmail domain.

@SomMeri
Copy link
Member

SomMeri commented Dec 4, 2013

Does it mean that

.class .class .class .class {
 a:b;
}
.extension:extend(.class all) {}

should generate following 16 selectors?

.class .class .class .class,

.extension .class .class .class,
.class .extension .class .class,
.class .class .extension .class,
.class .class .class .extension,

.extension .extension .class .class,
.extension .class .extension .class,
.extension .class .class .extension,
.class .extension .extension .class,
.class .extension .class .extension,
.class .class .extension .extension,

.extension .extension .extension .class,
.extension .extension .class .extension,
.extension .class .extension .extension,
.class .extension .extension .extension,

.extension .extension .extension .extension {
  a: b;
}

Now it generates

.class .class .class .class,
.extension .extension .extension .extension {
  a: b;
}

@calvinjuarez
Copy link
Member Author

In that case, yeah, I'd expect the 16. It may be verbose CSS, but it follows the expected behavior for LESS.

@seven-phases-max
Copy link
Member

So a minimal example for the issue would be:

.class .class {/* stuff */}
.extension:extend(.class all) {}

^ should result in 4 selectors via combinatorial explosion (not 2 as it is now).

Also referencing #1952 and #1641. In fact all these three issues are the same thing under the hood - and here's the key.

@matthew-dean
Copy link
Member

matthew-dean commented Aug 19, 2019

What should be the combinatorial cap? Should anything over 2 just do global search/replace within the selector list? (This was what I was leaning towards.) Should any explosion be allowed?

@matthew-dean
Copy link
Member

matthew-dean commented Aug 19, 2019

This extend branch does a 2-match branch. See: https://github.com/matthew-dean/less.js/blob/extend/test/css/extend.css#L73-L78

I haven't yet figured out the math for n number of match combinations, but it should work in theory. I should be able to just refactor here: https://github.com/matthew-dean/less.js/blob/extend/lib/less/visitors/extend-visitor.js#L190-L191

But what I want to know is: should there be a limit?

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

5 participants