Skip to content
New issue

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

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

Already on GitHub? # to your account

@extend with wrapped selectors has combinatorial explosion of selectors #2399

Closed
uberska opened this issue May 22, 2017 · 10 comments
Closed

Comments

@uberska
Copy link
Member

uberska commented May 22, 2017

@extend with wrapped selectors has combinatorial explosion of selectors

input.scss

.thing {
	color: black;
}

.a,
.b,
.c,
.d,
.e {
	&:not(.thing) { @extend .thing; }
}

libsass 3.5.0.beta.3-20-g9eb7b333 via SourceMap Inspector 1

.thing, .a:not(.thing):not(.a:not(.thing)):not(
.b:not(.thing):not(.a:not(.thing)):not(
.b:not(.thing)):not(
.c:not(.thing):not(.a:not(.thing)):not(
.b:not(.thing)):not(
.c:not(.thing)):not(
.d:not(.thing):not(.a:not(.thing)):not(
.b:not(.thing)):not(
.c:not(.thing)):not(
.d:not(.thing)):not(
.e:not(.thing):not(.a:not(.thing)):not(
.b:not(.thing)):not(
.c:not(.thing)):not(
.d:not(.thing)):not(
.e:not(.thing)))):not(
.e:not(.thing):not(.a:not(.thing)):not(
.b:not(.thing)):not(
.c:not(.thing)):not(
.d:not(.thing)):not(
.e:not(.thing)))):not(
.d:not(.thing):not(.a:not(.thing)):not(
.b:not(.thing)):not(
.c:not(.thing)):not(
.d:not(.thing)):not(
.e:not(.thing))):not(
.e:not(.thing):not(.a:not(.thing)):not(
.b:not(.thing)):not(
.c:not(.thing)):not(
.d:not(.thing)):not(
.e:not(.thing)))):not(
.c:not(.thing):not(.a:not(.thing)):not(
.b:not(.thing)):not(
.c:not(.thing)):not(
.d:not(.thing)):not(
.e:not(.thing))):not(
.d:not(.thing):not(.a:not(.thing)):not(
.b:not(.thing)):not(
.c:not(.thing)):not(
.d:not(.thing)):not(
.e:not(.thing))):not(
.e:not(.thing):not(.a:not(.thing)):not(
.b:not(.thing)):not(
.c:not(.thing)):not(
.d:not(.thing)):not(
.e:not(.thing))),
.b:not(.thing):not(.a:not(.thing)):not(
.b:not(.thing)):not(
.c:not(.thing)):not(
.d:not(.thing)):not(
.e:not(.thing)),
.c:not(.thing):not(.a:not(.thing)):not(
.b:not(.thing)):not(
.c:not(.thing)):not(
.d:not(.thing)):not(
.e:not(.thing)),
.d:not(.thing):not(.a:not(.thing)):not(
.b:not(.thing)):not(
.c:not(.thing)):not(
.d:not(.thing)):not(
.e:not(.thing)),
.e:not(.thing):not(.a:not(.thing)):not(
.b:not(.thing)):not(
.c:not(.thing)):not(
.d:not(.thing)):not(
.e:not(.thing)) {
  color: black; }

ruby sass 3.4.21 via SassMeister

.thing, .a:not(.thing),
.b:not(.thing),
.c:not(.thing),
.d:not(.thing),
.e:not(.thing) {
  color: black;
}
@mgreter
Copy link
Contributor

mgreter commented May 22, 2017

This seems to be a duplicate of #2055 (in some other form)

@uberska
Copy link
Member Author

uberska commented May 22, 2017

Thanks for the quick comment! I saw that issue, but didn't realize it was the same thing.

We're trying to update our codebase from 3.0 -> 3.5.0.beta3. This issue causes quite a bit of bloat in our css. Where I have .a through .e in the example, we actually have 16 selectors.

From some of the other issues I found through the link you provided, it sounds like support for extending wrapped selectors isn't fully supported yet. Given that we're coming from 3.0 and haven't had extend for wrapped selectors yet, one outcome that could be useful for us would be to turn it off until the implementation is fully supported. Would you accept a pull request in libsass and node-sass for a flag to disable extends on wrapped selectors?

@mgreter
Copy link
Contributor

mgreter commented May 22, 2017

No, because basic stuff works and we don't want to break that. Maybe if specifically targeting :not, but I think a correct patch would be hard to come up. As it is difficult to get correct :not extend behavior. I guess you might be better off with some specific post-processing in this case, if the bloat really bothers you. Not even sure why you would want to disable that feature, couldn't you just remove the line in scss then? Of course we always welcome PRs that improve libsass and do not break any existing spec tests.

@mgreter
Copy link
Contributor

mgreter commented May 22, 2017

Took me 2 hours to come up with a hotfix to at least hide nested :not selectors. This will probably cover about 70% of real world use cases (including this one) and should not give any "wrong negatives". Will take me some moments to create needed spec test and PR ...

@uberska
Copy link
Member Author

uberska commented May 22, 2017

Wow, thanks for the quick turnaround, @mgreter ! We'll try a new build with libsass master.

@uberska
Copy link
Member Author

uberska commented May 30, 2017

With master, our problem seems to be fixed. Thanks @mgreter !

@mgreter
Copy link
Contributor

mgreter commented May 30, 2017

Cool, great, thanks for the feedback!

@marticongost
Copy link

marticongost commented Jul 4, 2017

I have noticed a regression on 3.4.5 that I suspect may be related to this change.

This is the SASS source:

#searchForm {
    flex: 1 1 auto;

    :host(:not([searchEnabled='true'])) & {
        display: none;
    }
}

In 3.4.3, libsass produced the following CSS:

#searchForm {
  flex: 1 1 auto; }
  :host(:not([searchEnabled='true'])) #searchForm {
    display: none; }

Whereas on 3.4.5 I get this:

#searchForm {
  flex: 1 1 auto; }
   #searchForm {
    display: none; }

Notice the change in the selector for the inner rule. If I remove :not() from the original source 3.4.5 works as expected, so the problem does seem to be related to the handling of :not():

#searchForm {
  flex: 1 1 auto; }
  :host([searchEnabled='true']) #searchForm {
    display: none; }

@mgreter
Copy link
Contributor

mgreter commented Jul 4, 2017

@marticongost this is not reproducable with latest master and gives your expected result.

@marticongost
Copy link

The online tool seems to be using 3.5b. Could this have been fixed by a later change in master (5a7dbc8)? I can certainly reproduce it in 3.4.5. I'm using the Python bindings.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

3 participants