Skip to content

Extend within (reference) imported files. #1851

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

Open
donaldpipowitch opened this issue Feb 5, 2014 · 19 comments
Open

Extend within (reference) imported files. #1851

donaldpipowitch opened this issue Feb 5, 2014 · 19 comments

Comments

@donaldpipowitch
Copy link

Say I have three files:

// fileA.less
.test-a {
  color: red;
}
// fileB.less
@import (reference) "fileA";

.test-b {
  background-color: green;
  &:extend(.test-a all);
}
// fileC.less
@import (reference) "fileB";

.test-c {
  &:extend(.test-b all);
}

What I expect, if I compile fileC.less (comments only to aid the understanding):

// .test-a,
// .test-b,
.test-c {
  color: red;
}

// .test-b,
.test-c {
  background-color: green;
}

What I get:

.test-b,
.test-c {
  color: red;
}
.test-c {
  background-color: green;
}

.test-b leaks in!

@seven-phases-max
Copy link
Member

.test-b leaks in!

This is just how extend works currently. It is questionabe if .test-b should appear in the output (i.e. ideally it may not output however there's also no reason why it must not), but the whole snippet looks more like "trying to use (reference) as a hack to achieve #1177".

@lukeapage
Copy link
Member

I think its a bug, but would need to work out what's causing it to be sure.

@seven-phases-max
Copy link
Member

Ah., sorry, my bad. I did miss that with the one level @import it actually supresses the output of the extended class (I was under impression that I have tried this as #1177 workaround before and it did not work... But aparently my test was somewhat wrong :)

@SomMeri
Copy link
Member

SomMeri commented Feb 17, 2014

I think that the expected output in the above should be:

.test-c {
  color: red;
}
.test-c {
  background-color: green; // changed from color: red;
}

@donaldpipowitch
Copy link
Author

You're right. That is a typo in my example. I will fix this.

@bassjobsen
Copy link
Contributor

as far as i understood, the problem also occurs without nesting:

@import (reference) "testextend.less";

with testextend.less contains:

.extendthis {
notvisible: notvisiblevalue;
}
.a:extend(.extendthis){}
.c {
notvisible: notvisiblevalue;
}

will be compiled into:

.a {
  notvisible: notvisiblevalue;
}

Where i expected no output at all. Tested with 1.6.3 and 1.7.0

@donaldpipowitch
Copy link
Author

Any progress on this and the related issues?

@donaldpipowitch
Copy link
Author

@lukeapage Any chance to get this fixed for version 2.0.0? This would be incredible useful if you distribute very modular files via npm or Bower.

@lukeapage
Copy link
Member

Im not going to delay v2 till this is fixed... It might be easy to fix though..

@lukeapage
Copy link
Member

I would create a test case, then debug extend visitor, look at where it creates the new selector and make sure its referenced field is only set to true if the extend is referenced (which it wont be if ibside a refrenced import).
but not entirely sure, its been a while sjnce working on those features.

@donaldpipowitch
Copy link
Author

Thank you for the effort.

@bassjobsen
Copy link
Contributor

i wonder if this issue can be fixed by using the following lines of code in parser/parser.js:

if(typeof elements[0].currentFileInfo.reference === "undefined"
|| elements[0].currentFileInfo.reference === false)
{
    extend = new(tree.Extend)(new(tree.Selector)(elements), option, index);
    if (extendList) { extendList.push(extend); } else { extendList = [ extend ]; }
}

The above than should replace the lines:

    extend = new(tree.Extend)(new(tree.Selector)(elements), option, index);
    if (extendList) { extendList.push(extend); } else { extendList = [ extend ]; }

@lukeapage
Copy link
Member

No, because that would stop you extending the result of an extend through a
reference.

@bassjobsen
Copy link
Contributor

Well, i see. Thanks.
It seems possible to fix this issue in the to-css-vistor.js inn the filter function:

                .filter(function(p) {
                    var i;
                    if (p[0].elements[0].combinator.value === ' ') {
                        p[0].elements[0].combinator = new(tree.Combinator)('');
                    }
                    for(i = 0; i < p.length; i++) {
                        if(
                        typeof p[i].elements[0].currentFileInfo.reference === "boolean" && 
                        p[i].elements[0].currentFileInfo.reference &&
                        ) return false;
                        if (p[i].getIsReferenced() && p[i].getIsOutput()) {
                            return true;
                        }
                    }
                    return false;
                });

But that left other issues with mixins in the referenced files un-fixed; mixins called from the main file (#1979) and mixins with nested selectors (#1968). Probably this rules should get the referenced state of the caller instead of that from the mixin.

@assadtony
Copy link

Any updates on this issue? :)

@SomMeri SomMeri self-assigned this Oct 11, 2015
SomMeri pushed a commit to SomMeri/less-rhino.js that referenced this issue Nov 20, 2015
- refactored how import reference works
- refactored to-css-visitor (this is side product, it was getting
  complicated)
- fixes issues less#1851, less#1896, less#1878, less#2716, less#1968, less#2162 (same as less#1896)
@SomMeri
Copy link
Member

SomMeri commented Dec 7, 2015

This should be fixed by #2729 .

@SomMeri SomMeri closed this as completed Dec 7, 2015
@donaldpipowitch
Copy link
Author

Just wow!

@SomMeri
Copy link
Member

SomMeri commented Jan 31, 2016

Re-opening, because it now compiles into

.test-c {
  background-color: green;
}

instead of

.test-c {
  color: red;
}

.test-c {
  background-color: green;
}

@SomMeri SomMeri reopened this Jan 31, 2016
@matthew-dean
Copy link
Member

Okay, if you get a fix for it, we can do a bugfix release soon.

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

8 participants
@lukeapage @matthew-dean @SomMeri @bassjobsen @donaldpipowitch @assadtony @seven-phases-max and others