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

.Mixin() within .Mixin results in error #413

Closed
tyler-dot-earth opened this issue Oct 3, 2011 · 18 comments
Closed

.Mixin() within .Mixin results in error #413

tyler-dot-earth opened this issue Oct 3, 2011 · 18 comments

Comments

@tyler-dot-earth
Copy link

Less code like this:

.container() {
    // css properties
}

.container {
    .container();
}

... Results in an error when compiled. It seems to be because they share the same name, although they are different (one is a mixin with empty arguments and one is just a class).

I'm using the ruby less gem to compile on Ubuntu.

@ChrisCinelli
Copy link

Easy fix: use a different name ;-)

@tyler-dot-earth
Copy link
Author

Well, I'm trying to create the option of letting the user have an unsemantic ruleset if they'd like it and the unsemantic ruleset would have the same code in .container as the .container() mixin -- so changing the name isn't particularly an option (or "fix").

@cloudhead
Copy link
Member

As the compiler sees it, they are the same. Classes in LESS are mixins, and mixins with no parameters are classes.

@lancehunt
Copy link

I just ran into this same issue when parsing the .clearfix() mixin in bootstrap using lessc and nodejs(v0.58)

I commented on their change that caused this, here:
twbs/bootstrap@39aca91

It would be nice if less.js at least gave a more descriptive error rather than just "undefined" when this happens. Or better yet, why not just create a hashmap to alias class names and mixins differently.

@n0nick
Copy link

n0nick commented Apr 4, 2012

+1 on this one.
If this is a syntax error, less.js should display it, and not just crash.

This is especially critical because this exact problem caused WEBrick to crash as well (trying to serve the less file through Rails' assets pipelines)

@krulik
Copy link

krulik commented Apr 18, 2012

I get an error with the following code:

@direction: ltr;

.left(@v) when (@direction = ltr) { left: @v; }
.left(@v) when (@direction = rtl) { right: @v; }
.right(@v) when (@direction = ltr) { right: @v; }
.right(@v) when (@direction = rtl) { left: @v; }

.carousel-control {
  /* ..css.. */

  &.right {
    .left(auto);
    .right(15px);
  }

}

I don't want to change the mixin name because its a convention and maps one-to-one with the corresponding CSS property. I also can't change the class name because its a 3rd party framework (bootstrap), that I'm porting to RTL support.

So I'm +1-ing it :)

@OlsonDev
Copy link

I just ran into this. I don't agree with the syntax in the documentation:

.wrap () {
  text-wrap: wrap;
  white-space: pre-wrap;
  white-space: -moz-pre-wrap;
  word-wrap: break-word;
}

pre { .wrap }

I think parametric mixins taking no arguments should be invoked as if they were a function. .wrap and .wrap() should not be equivalent. Use two hashmaps if you need to. I realize this is a breaking change but the current behavior is counter-intuitive. Sure, there are plenty of languages out there where properties and methods can't share the same name or getters/setters can't have different accessibility levels. But, this just feels wrong. I didn't even realize I could do .wrap when only .wrap() is defined until I ran into a stack overflow error and resorted to your documentation. I've always used .wrap() to mixin something I wanted to hide from the CSS output.

@lukeapage
Copy link
Member

I'm planning on just fixing this so it won't recurse/pull in a mixin on the
current path. Then everyones happy?

@OlsonDev
Copy link

I don't agree with it but I'll take it over nothing.

@matthew-dean
Copy link
Member

Can you explain / give an example of the different output you expect?

On 2012-10-24, at 7:56 AM, Josh Olson notifications@github.com wrote:

I just ran into this. I don't agree with the syntax in the documentation:

.wrap () {
text-wrap: wrap;
white-space: pre-wrap;
white-space: -moz-pre-wrap;
word-wrap: break-word;
}

pre { .wrap }

I think parametric mixins taking no arguments should be invoked as if they
were a function. .wrap and .wrap() should not be equivalent. Use two
hashmaps if you need to. I realize this is a breaking change but the
current behavior is counter-intuitive. Sure, there are plenty of languages
out there where properties and methods can't share the same name or
getters/setters can't have different accessibility levels. But, this just
feels wrong. I didn't even realize I could do .wrap when only .wrap() is
defined until I ran into a stack overflow error and resorted to your
documentation. I've always used .wrap() to mixin something I wanted to hide
from the CSS output.


Reply to this email directly or view it on
GitHubhttps://github.com//issues/413#issuecomment-9742516.

@OlsonDev
Copy link

@MatthewDL, I would expect an error in the example provided in the documentation. .wrap as a class (to be output & usable as a mixin) was never defined; .wrap() was. Proper usage would therefore be:

.wrap() {
  text-wrap: wrap;
  white-space: pre-wrap;
  white-space: -moz-pre-wrap;
  word-wrap: break-word;
}

pre { .wrap(); }

@matthew-dean
Copy link
Member

Ohhhh.... Yes, I get what you mean.

.wrap should theoretically bring up a class definition. .wrap() should bring in a mixin definition.

I think you make a good point, and I don't disagree, but it's unfortunately a breaking change. And changing the behavior I don't feel is advantageous enough to force that change. For now, I'd say just don't have mixin definitions clash with class definitions. It's documented well enough that currently, the parentheses are not required.

Also, I wonder if this is a case for LESS to have "warnings" generated and not just errors.

@matthew-dean
Copy link
Member

Having said that... I did just see @krulik's example. Hmm... @agatronic, your thoughts?

@lukeapage
Copy link
Member

@MatthewDL changing this is a massive breaking change.. lots of people rely on the parenthesis being optional. in @krulik's example it shouldn't match the mixin without parenthesis any more anyway because the agrument matching has changed.

I agree it may be a good place for a warning, but we don't have a mechanism to give those at the moment.

playing around with @krulik's example I still get issues.. will continue investigating.

@lukeapage lukeapage reopened this Oct 24, 2012
@lukeapage
Copy link
Member

ok, now even this is fine.. so you would have to have 2 mixins without parameters (one with, one without ()) where you were calling the mixin from more than just inside itself.. in this case I think it makes the less a lot clearer to rename one of the mixins.

@direction: ltr;

.left(@v) when (@direction = ltr) { left: @v; }
.left(@v) when (@direction = rtl) { right: @v; }
.right(@v) when (@direction = ltr) { right: @v; }
.right(@v) when (@direction = rtl) { left: @v; }

.carousel-control {
  .right {
    .left(auto);
    .right(15px);
  }
  .right(10px);
}

@krulik
Copy link

krulik commented Oct 24, 2012

@agatronic +1 for addressing this.
I agree in this case it's clearer to rename one of them to remove confusion about what mixin you're calling. These edge case are where both the power and shortcoming of the language meet - the ability to use implicit mixins. This is a tricky subject. Maybe this is why SASS didn't implement such a feature.
Another edge case is when you want a class to server both as a library class you can include in other .less files and as a utility class, such as .clearfix for example. If you import it in several files you've got it as many times. I ended up having two versions of this class (and several more such as this one) - one for a "real" class and one for an "abstract" class. You can see this Stack Overflow question for more details:
http://stackoverflow.com/questions/10185508/less-css-import-rules-without-styles?rq=1

@Soviut
Copy link

Soviut commented Oct 25, 2012

This is why I always make sure to call mixins with brackets even without arguments. That, or prefix them with an underscore to make it clear they're mixins.

@Soviut
Copy link

Soviut commented Oct 28, 2012

To reiterate what I said in #1007

(Assuming this can't be solved with scope checking) I feel this is suitable for a breaking change in LESS 1.4. Enforcing it in 1.3.2 might be pushing it seeing as it would be a point (point-point?) release as of right now.

stefanklug pushed a commit to stefanklug/carto that referenced this issue Jan 27, 2019
…SON variant is used and then either output directly or afterwards converted to XML, ref less#413
stefanklug pushed a commit to stefanklug/carto that referenced this issue Jan 27, 2019
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

10 participants