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

When using an @import (reference), mixins that contain an & selector get added to the compiled output improperly #1968

Closed
bcullman opened this issue Apr 12, 2014 · 30 comments

Comments

@bcullman
Copy link

When I import a less file with the (reference) directive, I expect to see no CSS output from that file.

However, I am seeing content being output from the imported file when a mixin contains an & selector.

mixin.less

.mixin-no-parent-selector() {
  background-color: red;
}
.mixin-with-parent-selector() {
  &:first-child{ //  USING AN & SELECTOR HERE
    background-color: blue;
 }
}

less-for-reference.less

.container-1 {
   .mixin-no-parent-selector();
}
.container-2{
   .mixin-with-parent-selector()
}

style.less

/*!
 * styles.less
 */
@import "mixin.less";
@import (reference) "less-for-reference.less";

Expected Output

/*!
 * styles.less
 */

Actual Output

/*!
 * styles.less
 */

.container-2:first-child{
    background-color: blue;
}
@bcullman bcullman changed the title When importing using reference, mixins that container an & selector get added to the compiled output When @importing using (reference), mixins that contain a & selector get added to the compiled output Apr 12, 2014
@bcullman bcullman changed the title When @importing using (reference), mixins that contain a & selector get added to the compiled output When using @import (reference), mixins that contain an & selector get added to the compiled output Apr 12, 2014
@bcullman bcullman changed the title When using @import (reference), mixins that contain an & selector get added to the compiled output When using an @import (reference), mixins that contain an & selector get added to the compiled output improperly Apr 12, 2014
@thomaswelton
Copy link

I can replicate this error. Using twitter bootstrap and importing it as a reference generates a lot of output I did not expect to see

@bcullman
Copy link
Author

Yes, I discovered this when working with bootstrap too. I built this example to simplify the issue down a bit.

@seven-phases-max
Copy link
Member

I can't reproduce the CSS output of your example with Less 1.7.0.

As for Bootstrap, the non empty reference output is the result of extend directives (&:extend(.clearfix all);, &:extend(.btn-*); etc.) which is a different issue (see #1851, #1878).

@bcullman
Copy link
Author

Here is the same issue using bootstrap files, with one tweak to remove the 2 extend statements you mention are problematic.

style.less

@import "bootstrap-3.1/less/variables.less";
@import "bootstrap-3.1/less/mixins.less"; //modified to comment out &:exend() statements

@import (reference) "bootstrap-3.1/less/buttons.less";

Output should be nothing. However, I get

.btn-default:hover,
.btn-default:focus,
.btn-default:active,
.btn-default.active,
.open .dropdown-toggle.btn-default {
  color: #333333;
  background-color: #ebebeb;
  border-color: #adadad;
}
.btn-default:active,
.btn-default.active,
.open .dropdown-toggle.btn-default {
  background-image: none;
}
.btn-default.disabled,
.btn-default[disabled],
fieldset[disabled] .btn-default,
.btn-default.disabled:hover,
.btn-default[disabled]:hover,
fieldset[disabled] .btn-default:hover,
.btn-default.disabled:focus,
.btn-default[disabled]:focus,
fieldset[disabled] .btn-default:focus,
.btn-default.disabled:active,
.btn-default[disabled]:active,
fieldset[disabled] .btn-default:active,
.btn-default.disabled.active,
.btn-default[disabled].active,
fieldset[disabled] .btn-default.active {
  background-color: #ffffff;
  border-color: #cccccc;
}
.btn-default .badge {
  color: #ffffff;
  background-color: #333333;
}
.btn-primary:hover,
.btn-primary:focus,
.btn-primary:active,
.btn-primary.active,
.open .dropdown-toggle.btn-primary {
  color: #ffffff;
  background-color: #3276b1;
  border-color: #285e8e;
}
.btn-primary:active,
.btn-primary.active,
.open .dropdown-toggle.btn-primary {
  background-image: none;
}
.btn-primary.disabled,
.btn-primary[disabled],
fieldset[disabled] .btn-primary,
.btn-primary.disabled:hover,
.btn-primary[disabled]:hover,
fieldset[disabled] .btn-primary:hover,
.btn-primary.disabled:focus,
.btn-primary[disabled]:focus,
fieldset[disabled] .btn-primary:focus,
.btn-primary.disabled:active,
.btn-primary[disabled]:active,
fieldset[disabled] .btn-primary:active,
.btn-primary.disabled.active,
.btn-primary[disabled].active,
fieldset[disabled] .btn-primary.active {
  background-color: #428bca;
  border-color: #357ebd;
}
.btn-primary .badge {
  color: #428bca;
  background-color: #ffffff;
}
.btn-success:hover,
.btn-success:focus,
.btn-success:active,
.btn-success.active,
.open .dropdown-toggle.btn-success {
  color: #ffffff;
  background-color: #47a447;
  border-color: #398439;
}
.btn-success:active,
.btn-success.active,
.open .dropdown-toggle.btn-success {
  background-image: none;
}
.btn-success.disabled,
.btn-success[disabled],
fieldset[disabled] .btn-success,
.btn-success.disabled:hover,
.btn-success[disabled]:hover,
fieldset[disabled] .btn-success:hover,
.btn-success.disabled:focus,
.btn-success[disabled]:focus,
fieldset[disabled] .btn-success:focus,
.btn-success.disabled:active,
.btn-success[disabled]:active,
fieldset[disabled] .btn-success:active,
.btn-success.disabled.active,
.btn-success[disabled].active,
fieldset[disabled] .btn-success.active {
  background-color: #5cb85c;
  border-color: #4cae4c;
}
.btn-success .badge {
  color: #5cb85c;
  background-color: #ffffff;
}
.btn-info:hover,
.btn-info:focus,
.btn-info:active,
.btn-info.active,
.open .dropdown-toggle.btn-info {
  color: #ffffff;
  background-color: #39b3d7;
  border-color: #269abc;
}
.btn-info:active,
.btn-info.active,
.open .dropdown-toggle.btn-info {
  background-image: none;
}
.btn-info.disabled,
.btn-info[disabled],
fieldset[disabled] .btn-info,
.btn-info.disabled:hover,
.btn-info[disabled]:hover,
fieldset[disabled] .btn-info:hover,
.btn-info.disabled:focus,
.btn-info[disabled]:focus,
fieldset[disabled] .btn-info:focus,
.btn-info.disabled:active,
.btn-info[disabled]:active,
fieldset[disabled] .btn-info:active,
.btn-info.disabled.active,
.btn-info[disabled].active,
fieldset[disabled] .btn-info.active {
  background-color: #5bc0de;
  border-color: #46b8da;
}
.btn-info .badge {
  color: #5bc0de;
  background-color: #ffffff;
}
.btn-warning:hover,
.btn-warning:focus,
.btn-warning:active,
.btn-warning.active,
.open .dropdown-toggle.btn-warning {
  color: #ffffff;
  background-color: #ed9c28;
  border-color: #d58512;
}
.btn-warning:active,
.btn-warning.active,
.open .dropdown-toggle.btn-warning {
  background-image: none;
}
.btn-warning.disabled,
.btn-warning[disabled],
fieldset[disabled] .btn-warning,
.btn-warning.disabled:hover,
.btn-warning[disabled]:hover,
fieldset[disabled] .btn-warning:hover,
.btn-warning.disabled:focus,
.btn-warning[disabled]:focus,
fieldset[disabled] .btn-warning:focus,
.btn-warning.disabled:active,
.btn-warning[disabled]:active,
fieldset[disabled] .btn-warning:active,
.btn-warning.disabled.active,
.btn-warning[disabled].active,
fieldset[disabled] .btn-warning.active {
  background-color: #f0ad4e;
  border-color: #eea236;
}
.btn-warning .badge {
  color: #f0ad4e;
  background-color: #ffffff;
}
.btn-danger:hover,
.btn-danger:focus,
.btn-danger:active,
.btn-danger.active,
.open .dropdown-toggle.btn-danger {
  color: #ffffff;
  background-color: #d2322d;
  border-color: #ac2925;
}
.btn-danger:active,
.btn-danger.active,
.open .dropdown-toggle.btn-danger {
  background-image: none;
}
.btn-danger.disabled,
.btn-danger[disabled],
fieldset[disabled] .btn-danger,
.btn-danger.disabled:hover,
.btn-danger[disabled]:hover,
fieldset[disabled] .btn-danger:hover,
.btn-danger.disabled:focus,
.btn-danger[disabled]:focus,
fieldset[disabled] .btn-danger:focus,
.btn-danger.disabled:active,
.btn-danger[disabled]:active,
fieldset[disabled] .btn-danger:active,
.btn-danger.disabled.active,
.btn-danger[disabled].active,
fieldset[disabled] .btn-danger.active {
  background-color: #d9534f;
  border-color: #d43f3a;
}
.btn-danger .badge {
  color: #d9534f;
  background-color: #ffffff;
}

@seven-phases-max
Copy link
Member

@bcullman

OK, I see now. The problem there is that (reference) is not inheritable across multiple files. If mixins.less is imported from buttons.less it inherits the reference modifier so the output is empty. But when mixins.less is included directly in the master file with no sign of reference, all classes generated there get into the output. (Workaround is obvious: put (reference) on mixins.less too).

Suggestion: add @import "mixin.less"; to the style.less in your original example. (Otherwise it's not that evident how to reproduce your results. Many developers follow the "import things where they are used" guidelines and that way (i.e. mixin.less imported within less-for-reference.less) it works as expected).

@bcullman
Copy link
Author

@seven-phases-max

That does work. I am willing to consider this matter closed.

@seven-phases-max
Copy link
Member

I think I'll reopen this since it's still an issue to keep in mind (I don't know how easy that would be to fix but at least let's consider if it's possible eventually).

@almeidap
Copy link

It seems that this issue also happens when compiling independent entry files. For example:

styles.less

@import "grid.less";

theme.less

@import (reference) "grid.less";

Then the compiled output of theme.less will contain all CSS rules instead of an expected empty output. Note that grid.less does not import theme.less and vice versa. Therefore, it seems that the second import inherits from the first one.

@seven-phases-max
Copy link
Member

@almeidap Do you mean the case when you compile some other file that imports both styles.less and theme.less? If so they're no longer "independent entry files". Or do you mean something else? Could you please provide complete example?

@almeidap
Copy link

almeidap commented May 2, 2014

@seven-phases-max Ok, just discovered that lessc can not compile (or no more compile) multiple files at one. It seems that the issue comes from the grunt-contrib-less task (https://github.com/gruntjs/grunt-contrib-less/):

module.exports = function (grunt) {

    /*jslint node: true */
    'use strict';

    // Project configuration
    grunt.initConfig({

        // LESS (https://github.com/gruntjs/grunt-contrib-less)
        less: {
            css: {
                files: [
                    {
                        expand: true,
                        cwd: 'less/',
                        src: 'styles.less',
                        dest: 'dist/css/',
                        ext: '.css'
                    }
                ]
            },
            themes: {
                files: [
                    {
                        expand: true,
                        cwd: 'themes/',
                        src: '**/*-theme.less',
                        dest: 'dist/themes/',
                        ext: '.css'
                    }
                ]
            }
        }
    });

    // Default task:
    grunt.registerTask('default', ['less']);
};

Should I report this issue to grunt-contrib-less or is there a possibility that Less may share context/state/variables while compiling successive independent files?

@seven-phases-max
Copy link
Member

@almeidap I don't see any issue you could report to grunt-contrib-less at all in your example. styles.less and *-theme.less files are not only compiled by independent instances of the compiler but also are run by independent grunt task "targets" hence they not only can't share anything but also should not share anything (simply by the definition of their independence :)

If you need them to re-use the same "state" either put that re-used stuff into both the same way (e.g. use @import (reference) "grid.less"; in both) or compile them as a single file (e.g. import styles.less from theme.less or vice-versa).

@almeidap
Copy link

almeidap commented Dec 5, 2014

@seven-phases-max I made more extensive tests and, as per version 1.7.2+, the issue only occurs when the & character is used to define selectors. Do you plan any fix for this in the version 1 branch?

@seven-phases-max
Copy link
Member

@almeidap

Do you plan any fix for this in the version 1 branch?

Well, I don't know if anything of reference is to be fixed soon (hence I set "lower priority" above, also counting that this issue is more about "messy imports" and a workarounds are pretty simple). Either way 1.x branch won't be updated anymore of course.

@gdelhumeau
Copy link
Contributor

I hit the same problem in my use-case. I need to import a LESS file which performs a lot of imports (including bootstrap) in an other LESS file, just to be able to re-use the variables of the first file. As a result, I get a lot of unwanted CSS rules, and I just cannot modify the first file to use your workaround.

Thanks,
Guillaume D. from XWiki.org

@seven-phases-max
Copy link
Member

just to be able to re-use the variables of the first file

To be able to re-use variables import just variables.less, don't import bootstrap.less and don't tease (reference). (Same for mixins).

@gdelhumeau
Copy link
Contributor

No, because in my use-case, some variables can be created in the main LESS file too (and actually I also need the bootstrap mix-ins). I really need to import this file and I cannot use the workaround.

@seven-phases-max
Copy link
Member

because in my use-case, some variables can be created in the main LESS file

So what? What your main Less file has to do with Bootstrap variables and mixins? Do it like this:

// your main Less file:

@import "bootstrap/variables.less";
@import "bootstrap/mixins.less";

// other imports:

// your variables:

@Synchro
Copy link
Member

Synchro commented Dec 16, 2014

This is exactly what I do - the big upside of this approach is that it means you can upgrade bootstrap without touching any of its less files.

@gdelhumeau
Copy link
Contributor

This is my use case (short story):

// file1.less
@import "bootstrap/bootstrap.less";
// ... other imports

// Some variables and some mixins there

This is compiled to file1.css that I use across my website.
I also have:

// File file2.less
@import (reference) "file1.less";
// Some rules using @variables and mixins defined by bootstrap AND file1.less

This is compiled to file2.css, that I only use in some pages in my website.

I cannot change the way it works. My use-case is a wiki containing a lot of extensions and the ability to add some LESS code in any wiki page, that can use bootstrap mixins and so on. So the workaround you proposed is not possible there.

More information: http://design.xwiki.org/xwiki/bin/view/Proposal/LESSModuleImprovements and http://extensions.xwiki.org/xwiki/bin/view/Extension/Skin+Extension+Plugin

And I hit this bug (or maybe it is only #1878).

Thanks,
Guillaume

@seven-phases-max
Copy link
Member

This is my use case:

What you need is to split the front-end (e.g. the CSS) part of file1.less from the shared "config" part (i.e. Bootstrap overrides), e.g.:

// file1.less:
@import "bootstrap/bootstrap.less";
@import "bootstrap/file3.less"; // <- your shared stuff (incl. Bootstrap overrides)
// file3.less:
// Some variables and some mixins there
// file2.less:
@import "bootstrap/variables.less";
@import "bootstrap/mixins.less";
@import "file3.less";

It's important to realize that even if this (and similar) bug is fixed once it will be no guarantee it's not broken with further Bootstrap releases - simply because "bootstrap.less" is meant to generate CSS - so they never will bother if or if not it's compatible with whatever sort-of-kludge features that may try to hack it externally.

@gdelhumeau
Copy link
Contributor

Then I don't see what is the goal of the reference option.

@bcullman
Copy link
Author

I think the issue is that you should avoid mixing-in against compiled CSS (or, in your case what is the equivalent of compiled CSS as you are importing essentially a manifest-like LESS file).

This is how I layout my Less files - it might prove helpful to others. Because I segregate the base less files from bootstrap (into /third-party), and further differentiate between custom and override styles, I have been able to keep pace with bootstrap version updates with minimal disruption.

Directory Structure

  • /static
    • /third-party
      • /bootstrap-3.3.1 (unmodified content from bootstrap 3.3.1)
        • /less
          • variables.less (I do not use this file)
          • mixins.less
          • ...(all other bootstrap .less files)
        • /js (this is where all unedited js files from bootstrap reside)
    • /less
      • /corp
        • /global
          • styles.less (site style used throughout corp site)
        • /views
          • page1.less (site style for just page 1)
          • page2.less (site style for just page 2)
          • ... (all other per page .less files)
        • /shared
          • prerequisites.less (contains references to variables and mix-ins from all sources)
          • postrequisites.less (contains references to utils and responsive utils from all sources)
      • /corp-boostrap-3.3.1
        • /custom (contains customizations not found in bootstrap)
          • variables.less (custom variables.less not defined by bootstrap)
          • mixins.less (custom mixins not defined by bootstrap)
        • /override (contains overrides of bootstrap styles)
          • variables.less (same as variables.less from bootstrap, but with values for my corp)
          • mixins.less (overrides from bootstrap)

Files

Pre- and post- requisites.less

prerequisites.less
This is the file that facilitates sharing of variables and mixins with site styles and page styles.

@import (reference) "../../corp-bootstrap-3.3.1/override/variables.less";
@import (reference) "../../corp-bootstrap-3.3.1/custom/variables.less";
@import (reference) "../../../_third-party/bootstrap-3.3.1/less/mixins.less";
@import (reference) "../../corp-bootstrap-3.3.1/override/mixins.less";
@import (reference) "../../corp-bootstrap-3.3.1/custom/mixins.less";

postrequisites.less

@import "../../../_third-party/bootstrap-3.3.1/less/utilities.less";
@import "../../../_third-party/bootstrap-3.3.1/less/responsive-utilities.less";
@import "../../corp-bootstrap-3.3.1/override/responsive-utilities.less";

html page referenced .less files

styles.less (used on all pages)

//
// similar to bootstrap.less, but contains overrides and customizations.
//
@import "../shared/prerequisites.less"

// Reset and dependencies
@import "../../../_third-party/bootstrap-3.3.1/less/normalize.less";
@import "../../../_third-party/bootstrap-3.3.1/less/print.less";

// Core CSS 
@import "../../../_third-party/bootstrap-3.3.1/less/scaffolding.less";
@import "../../../_third-party/bootstrap-3.3.1/less/type.less";
... (other core CSS @imports from _third-party/)

// Components (only @import what is used by corp site)
@import "../../../_third-party/bootstrap-3.3.1/less/dropdowns.less";
... (other components @imports from _third-party/)

// Components w/ JavaScript (only @import what is used by corp site)
@import "../../../_third-party/bootstrap-3.3.1/less/modals.less";
@import "../../../_third-party/bootstrap-3.3.1/less/carousel.less";

// Components: Corp Customizations
@import "../../corp-bootstrap-3.3.1/custom/global.less";
@import "../../corp-bootstrap-3.3.1/custom/chrome.less";

// Components: Corp Override
@import "../../corp-bootstrap-3.3.1/override/grid.less";
@import "../../corp-bootstrap-3.3.1/override/type.less";
... (other @imports overrides)

@import "../shared/postrequisites.less"

page1.less (example of page-specific style)

@import "../shared/prerequisites.less"

... page css here

@import "../shared/postrequisites.less"

@almeidap
Copy link

@bcullman I suppose that any frontend developer that extends a CSS framework will end up with the same kind of structure and import ordering (even if yours sounds, imho, over-complicated and probably misses the point with one dedicated stylesheet per page).

The problem here, as @gdelhumeau spots it, is that the reference feature does not respect its contract as stated in the docs (http://lesscss.org/features/):

Use @import (reference) to import external files, but without adding the imported styles to the compiled output unless referenced.

Therefore, whatever messy imports I may be using, the @import (reference) statement line should output exactly zero characters. Otherwise, please, update the docs explaining why it's not the case if this issue does not deserve to be fixed.

@bcullman
Copy link
Author

@almeidap for the record, I don't have anywhere near one dedicated stylesheet per page. But when they are needed, I have a place to put them.

As for the issue at hand, I think that @seven-phases-max noted in an earlier answer to this that (reference) is not inheritable across multiple files.

This is why I posted how my files are structured. I never double (reference) because all my necessary references are in my prerequisites.less file, which is shared by my overall corp style as well as any individual page styles I need.

@seven-phases-max
Copy link
Member

@almeidap

Otherwise, please, update the docs explaining

You forgot the project is a volunteer-based open source one, did you? The docs are open source as well. So please, let's take it a bit more easy (the issue is marked as bug and my comments are mostly intended to help to avoid stepping into this bug with a bit more structured project files organization, that's it).

@almeidap
Copy link

@seven-phases-max For sure, I did not forget that less.js is a volunteer-based project and I am very grateful to the contributors. I just wanted to bring the focus back to the issue itself instead of not-so-useful (imo) discussions about the usage of imports.

I recently started refactoring our LESS resources as our framework hit the IE stylesheets limit when concatenated with other CSS project files. Fixing the (reference) import issue will help me to avoid duplicate selectors and better control outputted resources.

evalica added a commit to xwiki/xwiki-platform that referenced this issue Dec 18, 2014
…-looking with the Flamingo skin

- Because of the bug less/less.js#1968 and after the commit e9b2bfc , we cannot use :extend anymore
- Since I couldn't transform all the extends into mixins (failed for examples like '.dropdown-menu>li>a:hover', etc.) I copied the styling properties from Bootstrap
- In the future, if we fix the extend bug, we should clean this code (either of the properties or the extend comments)
@assadtony
Copy link

Any updates on this issue? :)

@SomMeri SomMeri self-assigned this Oct 11, 2015
@thybzi
Copy link

thybzi commented Oct 25, 2015

Wish that to be fixed. My .clearfix mixin runs many-many times on the same selector, because it contains &:after

@SomMeri
Copy link
Member

SomMeri commented Oct 26, 2015

I am working on import reference now, but I am not able to give out estimate.

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
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