sass / libsass

A C/C++ implementation of a Sass compiler
https://sass-lang.com/libsass
Other
4.33k stars 461 forks source link

Extended selectors wont compile correctly (with both regular and silent selectors) #146

Closed lmartins closed 9 years ago

lmartins commented 11 years ago

When you have something like:

%btn-style-default{
  background: green;
  &:hover{
    background: black;
  }
}

and extend this selector with:

button{
  @extend %btn-style-default;
}

should compile to:

button{
   background: green;
}
button:hover{
   backgroud: black
}

but instead it compiles to invalid (and obviously non-functional) css:

button {
  background: green; }
  %btn-style-default:hover {
    background: black; }

Any news on this?

lmartins commented 11 years ago

The only way I can get something similar is by using mixins, but that's far from ideal.

HamptonMakes commented 11 years ago

Just added a spec test for this example... https://github.com/hcatlin/sass-spec/commit/8a6a1c32164bbe3c69a0a87ab71bc34557d6c4e0

akhleung commented 11 years ago

Referencing #80. This feature is difficult to implement completely, so I can't give an ETA yet.

lmartins commented 11 years ago

@akhleung Sure, I get it. Thanks for all the hard work, wish I was able to help here.

lmartins commented 11 years ago

Just to add to this issue, i've noticed that even regular selectors wont extend correctly:

.f-row--defaults{

  margin-bottom: 1.5em;

  &:last-child{
    margin-bottom: 0;
    border: 1px solid red;
  }

}

and then extend any selector with: @extend .f-row--defaults;

It will only inerhit the base rule margin-bottom but not the child selector also declared on the selector used as extender.

lmartins commented 10 years ago

Sorry to ping this, but is there any estimation on when will this be available?

mshwery commented 10 years ago

+1. Really really incredibly important to the usefulness of sass.

emagnier commented 10 years ago

+1. Same for me.

JohnONolan commented 10 years ago

+1 this is a must-have

akhleung commented 10 years ago

We know this is an important feature and we're bumping up the priority on it ... it is, however, a very difficult feature to get completely right, but I'm hoping to consult with @nex3 in the near future to figure out how he did it in Ruby Sass.

lmartins commented 10 years ago

You guys are doing an awesome job. Thank your for that and for tolerating our impatience. I just can't use Ruby Sass now.

benfrain commented 10 years ago

Just to echo everything Luis said :)

benfrain commented 10 years ago

I've just added a bounty for this. I like to think it will buy those involved a beer/hot beverage/cocktail of choice whenever this gets finished. https://www.bountysource.com/issues/1057456-extend-classes-wont-compile-correctly-with-both-regular-and-silent-selectors/bounties

emagnier commented 10 years ago

I also added a bounty for this one. Hope this will help :)

akhleung commented 10 years ago

Working on the feature right now. 100% compatibility with Ruby Sass's @extend implementation is a monumental task, but I think I can at least address the specific use-cases in these tickets before the holidays.

emagnier commented 10 years ago

Cool, that's great news! Thanks for all the hard work!

dansowter commented 10 years ago

Really appreciate the hard work on this, guys. I'm porting over https://github.com/net-engine/trove to use libsass, and this seems to be one of the last issues.

akhleung commented 10 years ago

Okay, the two examples given are now working correctly on the @extend branch, which I'll try to merge in a couple of days.

This is not to say that the feature is done -- far from it! I'm just trying to get it working one chunk at a time.

jrabbe commented 10 years ago

@akhleung is there a branch I can try to see if it works with our Sass?

akhleung commented 10 years ago

The new code is in the @extend branch. It's a bit messy, but it you should be able to build it with the default makefile (I haven't updated the automake stuff yet).

lmartins commented 10 years ago

Sorry to introduce some noise to the ticket comments but I just wanted to give thanks to @akhleung for the hard work.

icasteleyn commented 10 years ago

Just wanted to add another example where @extends doesn't work properly. ".Sub.myInherit" should be ".myInherit.Sub"

Scss

.myBase { 
    text-decoration: none; 
}

.myInherit{
    @extend .myBase;
    line-height: 1.6;
}

.myBase.Sub{
    color: #123456;
}

SassLib

.myBase, .myInherit {
  text-decoration: none; }

.myInherit {
  line-height: 1.6; }

.myBase.Sub, .Sub.myInherit {
  color: #123456; }

Ruby Sass

.myBase, .myInherit {
  text-decoration: none; }

.myInherit {
  line-height: 1.6; }

.myBase.Sub, .myInherit.Sub {
  color: #123456; }
akhleung commented 10 years ago

Interesting; I'll look into why the order gets mixed up. (At least the output is still semantically correct, if I'm not mistaken.)

akhleung commented 10 years ago

Oh, also, I should mention that I've merged what I have so far into master, so @extend should work a little better than before.

mgol commented 10 years ago

Another test case (taken - and simplified - from our code base):

.common {
    color: red;
}
.outer {
    .inner {
        @extend .common;
    }
}
.outer2 {
    @extend .outer;
}

Correct output by Ruby Sass:

.common, .outer .inner, .outer2 .inner {
  color: red; }

libsass output:

.common, .outer .inner, .outer2 {
  color: red; }

The .inner part gets cut out.

mgol commented 10 years ago

And another, incorrect test case that gets properly shouted at by Ruby Sass:

.common {
    color: red;
}
.outer {
    .inner {
        @extend .common;
    }
}
.outer2 {
    @extend .middle;
}

(notice mismatched @extend .middle)

Ruby Sass gives:

Syntax error: ".outer2" failed to @extend ".middle".
              The selector ".middle" was not found.
              Use "@extend .middle !optional" if the extend should be able to fail.
        on line 10 of test.scss
  Use --trace for backtrace.

whereas libsass happily compiles that to the following:

.common, .outer .inner {
  color: red; }

Is there a proper place to report such issues? I have about 230 mismatched rules compared to the Ruby output, I can provide test cases but maybe it's not worth giving them all since many will be fixed when those already provided are fixed.

akhleung commented 10 years ago

@mzgol @extend is still under active development, and I'm testing against the ~250 or so cases in the Ruby implementation, so it's probably not necessary for you to copy your broken cases here (unless you feel that it's something really unusual). Thanks!

mgol commented 10 years ago

@akhleung OK, thanks for the info! Do you think the implementation requires a lot more work? Not pushing, just curious if we have to wait a little longer.

Anyway, you're doing awesome job here, I bumped the bounty. :)

akhleung commented 10 years ago

It's hard to say for sure, but I think @extend will still require a lot more effort -- that single feature has turned out to be much more sophisticated and subtle than I ever would have imagined when we started this project! Fortunately, @nex3 was kind enough to write up a description of his algorithm, and I've been reading the Ruby source as well, so I hope to get it all figured out in a month or two (depending on how much time I'm able to devote to this project, of course).

anotheruiguy commented 10 years ago

If anyone is looking for a summary of issues I encountered with libsass' version of extends, please see this post.

akhleung has an amazing undertaking, we appreciate all your work in this regard.

vkanwal commented 10 years ago

I switched to libsass for an existing enterprise project at work. All my @extends do not work. So, is there a workaround to get the speeds of libsass and work temporarily with extend till a good solution comes up.

mgol commented 10 years ago

@vaibhavkanwal Obviously not... If such a workaround existed it wouldn't really be a workaround but an implementation and it would be merged to libsass. Today you can choose between speed & reliable @extend.

vkanwal commented 10 years ago

@mzgol Sure. Makes sense. Eagerly await this fix.

akhleung commented 10 years ago

@vaibhavkanwal All of your @extends are breaking? I did push some new code a couple of months ago based on the algorithm in Ruby Sass -- perhaps that was premature. I'll see if I can make some improvements by early next week. Unfortunately, the full algorithm is complex enough such that doing it in bits and pieces won't produce reliable results; I need to port the whole thing, which I haven't had time for recently.

vkanwal commented 10 years ago

@akhleung I understand, thanks! I use @extend with silent classes. It did not give error for me but did not compile the code in output so styles were not applied. Hope that helps.

akhleung commented 10 years ago

Oh, also, if you could post a simple test case that is failing for you, that would be helpful. Thanks!

mlmorg commented 10 years ago

+1

halfdan commented 10 years ago

@akhleung Here's another example that works fine in Ruby Sass but doesn't produce the desired result in libsass: http://sassmeister.com/gist/9482759 (you can switch the compile mode on the top right corner).

Is there any ETA on the support for @extend?

mgol commented 10 years ago

I kindly remind anyone interested in it that there's an open bounty: https://www.bountysource.com/issues/1057456-extended-selectors-wont-compile-correctly-with-both-regular-and-silent-selectors-185 Currently $185, let's make it bigger! :-)

lunelson commented 10 years ago

Some of these use-cases are already working, based on what I can see on sassmeister; but a modification of @anotheruiguy's example—which is still broken—produces an interesting glitch:

%new-parent {
  border-width: 1px;
  border-style: solid;
  &.background-color {
    background-color: orange;
  }
  &.add-border {
    border: 1px solid red;
  }
}
.outer-box {
  border-width: 1px;
  border-style: solid; }
  .background-color.outer-box {
    background-color: orange; }
  .add-border.outer-box {
    border: 1px solid red; }

The silent class is not repeated here; but the parent selector reference is coming out in reverse order. It would still be valid since a compound class is valid in any order, but I wonder if it points to something deeper?

Anyway basic implementation of silent/placeholder classes—notwithstanding classes nested in the placeholder—is working for me so thanks @akhleung and everyone else.

bonfish commented 10 years ago

Hi! I am experiencing an @extend error using grunt-sass. The SCSS:

.module {
  padding: 10px;
  h3 {
    color: red; 
  }
}

.news {
  @extend .module;
}

should produce the CSS

.module, .news {
  padding: 10px;
}
.module h3, .news h3 {
  color: red;
}

but instead, it gives:

.module, .news {
    padding: 10px;
}
.module h3, .module .news {
    color: #FF0000;
}

Do I get this wrong, or is the culprit really the libsass in grunt-sass ?

HamptonMakes commented 10 years ago

Unfortunately, it's libsass doing that... ;/

On Wednesday, May 21, 2014, Aleksey notifications@github.com wrote:

Hi! I am experiencing an @extend https://github.com/extend error using grunt-sass. The SCSS:

.module { padding: 10px; h3 { color: red; } }

.news { @extend .module; }

should produce the CSS

.module, .news { padding: 10px; } .module h3, .news h3 { color: red; }

but instead, it gives:

.module, .news { padding: 10px; } .module h3, .module .news { color: #FF0000; }

Do I get this wrong, or is the culprit really the libsass in grunt-sass ?

Reply to this email directly or view it on GitHubhttps://github.com/hcatlin/libsass/issues/146#issuecomment-43721035 .

bonfish commented 10 years ago

@hcatlin thanks, though I was hoping to get the opposite answer.

SASS is sweet, the ability to work with it in such side tools as Grunt helps a lot. Extremely eager to see an update =)

Meanwhile, thank you guys so much for your work, wish all the best to the team! Aleksey.

Cinamonas commented 10 years ago

@bonfish Sass itself has nothing to do with this issue. Until libsass is fixed, you can use slower grunt-contrib-sass, which runs on Ruby.

KayLeung commented 10 years ago

@bonfish , or change your code a bit. libsass worked with non-nested @extend & %placeholder

Ilyes512 commented 10 years ago

@KayLeung I would rather use Ruby Sass, it just 2 seconds compilation at most?

I havent looked into the problem, but I find it strange that Ruby can do something that C cant? Ruby Sass is opensource right?

lunelson commented 10 years ago

@Ilyes512 ruby sass gets much slower than that on big projects. I've seen cases with over 5 seconds compile time. Which is just not workable, in this era where we're told we should 'design in the browser'. Speed is the basic reason for libsass. See @benfrain's article for reference http://benfrain.com/libsass-lightning-fast-sass-compiler-ready-prime-time/; and yes as @KayLeung said you can just simplify your use-cases for @extend. Basic non-nested usage and placeholder classes are already working.

HamptonMakes commented 10 years ago

Actually, Ruby Sass can take up to 2 hours to compile on very, very complex projects, but several minutes on regular large projects. Libsass is a new project to try and speed up Sass compilation, and ease both integration and memory usage.

Feel free to use the main Ruby-based project if it better fits your needs.

On Wednesday, May 28, 2014, Lu Nelson notifications@github.com wrote:

@Ilyes512 https://github.com/Ilyes512 ruby sass gets much slower than that on big projects. I've seen cases with over 5 seconds compile time. Which is just not workable, in this era where we're told we should 'design in the browser'. Speed is the basic reason for libsass. See @benfrainhttps://github.com/benfrain's article for reference http://benfrain.com/libsass-lightning-fast-sass-compiler-ready-prime-time/; and yes as @KayLeung https://github.com/KayLeung said you can just simplify your use-cases for @extend. Basic non-nested usage and placeholder classes are already working.

Reply to this email directly or view it on GitHubhttps://github.com/hcatlin/libsass/issues/146#issuecomment-44396470 .

cjcheshire commented 10 years ago

2 hours! Thank god i'm not on that project! :)

On 28 May 2014 16:31, Hampton Catlin notifications@github.com wrote:

Actually, Ruby Sass can take up to 2 hours to compile on very, very complex projects, but several minutes on regular large projects. Libsass is a new project to try and speed up Sass compilation, and ease both integration and memory usage.

Feel free to use the main Ruby-based project if it better fits your needs.

On Wednesday, May 28, 2014, Lu Nelson notifications@github.com wrote:

@Ilyes512 https://github.com/Ilyes512 ruby sass gets much slower than

that on big projects. I've seen cases with over 5 seconds compile time. Which is just not workable, in this era where we're told we should 'design in the browser'. Speed is the basic reason for libsass. See @benfrain< https://github.com/benfrain>'s

article for reference

http://benfrain.com/libsass-lightning-fast-sass-compiler-ready-prime-time/ ; and yes as @KayLeung https://github.com/KayLeung said you can just

simplify your use-cases for @extend. Basic non-nested usage and placeholder classes are already working.

Reply to this email directly or view it on GitHub< https://github.com/hcatlin/libsass/issues/146#issuecomment-44396470>

.

— Reply to this email directly or view it on GitHubhttps://github.com/hcatlin/libsass/issues/146#issuecomment-44413944 .

lmartins commented 10 years ago

2 hours might be extreme, but in my experience it is quite easy to get to 10sec average compilation times with ruby sass, which is unbearable to me. That is why I approach as @lunelson suggests, by being careful with some features and give for now on others. To me personally, the speed alone more than compensates for the missing (for now) features. Once you get used to have instant compilation times, you won't look back I promise.