Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

added failing test for #49 #431

Closed
wants to merge 5 commits into from

4 participants

@neonstalwart

test case to show problem described in #49

@neonstalwart

my naive first attempt was to try and remove duplicate rules when rendering a ruleset but this causes other tests to fail since they depend on the output containing duplicated rules. however, when the css is parsed, only the last rule will be effective so this wouldn't change the effect of the resulting css. in case anybody wants to see that approach, this is the diff. if @cloudhead likes this, i'll make a pull request of it. otherwise, i'll still be looking for other ways to fix this issue.

diff --git a/lib/less/tree/ruleset.js b/lib/less/tree/ruleset.js
index cc9a60a..3aee4ca 100644
--- a/lib/less/tree/ruleset.js
+++ b/lib/less/tree/ruleset.js
@@ -114,6 +114,7 @@ tree.Ruleset.prototype = {
             rulesets = [], // node.Ruleset instances
             paths = [],    // Current selectors
             selector,      // The fully rendered selector
+            names = {},    // used to remove duplicate names in rulesets
             rule;

         if (! this.root) {
@@ -161,6 +162,11 @@ tree.Ruleset.prototype = {
                         return s.toCSS(env);
                     }).join('').trim();
                 }).join(env.compress ? ',' : (paths.length > 3 ? ',\n' : ', '));
+                // only keep the last occurence for rules with the same name
+                rules = rules.reverse().filter(function (rule) {
+                    var name = rule.substr(0, rule.indexOf(':'));
+                    return !(name in names) && (names[name] = 1);
+                }).reverse();
                 css.push(selector,
                         (env.compress ? '{' : ' {\n  ') +
                         rules.join(env.compress ? '' : '\n  ') +
@neonstalwart neonstalwart mixins: don't duplicate rules with the same name (fixes #49)
 - this maintains the behavior where all mixins that matched the call
   are applied
 - if the result of applying the mixins produces rules with the same
   name then the last rule with that name wins
 - an alternative approach would be to only apply the last mixin that
   matches the call
1bee790
@neonstalwart

@cloudhead, there are alternative ways to approach this fix but this was the least different to how things work now. i'd be glad to take a different approach if you prefer.

neonstalwart added some commits
@neonstalwart neonstalwart the old dogs (IE) don't know these new tricks (reduceRight)
a741e39
@neonstalwart neonstalwart mixins: duplicating rules may be intended. only remove duplicate values
the following would be intentional and should be allowed:
	.foo {
		background: #fff;
		background: rgba(255, 255, 255, 0.8);
	}
06c3b06
@jeremyricketts

So, what's the word on this fix? This looks like a 5 month old pull request on a 2 year old bug. (#324 #49 #71)

@Kalyse

Agree. This need merging.
Just managed to knock of 6KB from a gzipped CSS build, and 100kb from the non-gzipped version because of this. Like Dojo, I have a very modular CSS base for all of my components, so neonstalwarts pull request was extremely useful.

@neonstalwart

@jeremyricketts this is not the only open pull request that solves bugs that have existed for a long time.

a number of people maintain their own forks of this project because of the lack of support for this library. if you have the opportunity to try an alternative library you should do it.

unfortunately i need to maintain some projects that don't have the opportunity to move away from less right now so i have to keep trying to get pull requests applied.

my current philosophy is "no less, no more" - i won't be using it on any new projects.

i've learned a valuable lesson that a permissive license is only a small part of being open source.

@Kalyse

@neonstalwart

I tried a pull of your fork of less earlier and checked out the multi-mixin branch. Unfortunately it was using the older version of less and also wasn't compatible with the newer way of requiring modules. ie dropping require.paths.

I've tried to copy what you did by using the rules.toCSS() but I couldn't get it to work. Something about compression module missing.

I see also that the version of less that Dojo uses is old too, so I couldn't drop my .less files into the /themes/* as my own custom replacement of claro/soria etc. I'm assuming that the other projects that you were referring to is Dojo, and my question is, 1) Do you have plans to bring your fork upto date with the latest version of Less? 2) Will Dojo ship with a later version of less seeing as it doesn't support some of the more useful mixin properties at the moment. (ie using when() ).

@neonstalwart

yes, dojo is one of the other projects i was referring to. the fork in the neonstalwart account is just for me to provide pull requests - i don't keep it up to date unless i need to open another pull request. if i recall correctly, the "usable" branch in my repo is the best one to try but i haven't touched it in a while. the fork that i use for work with my employer is at https://github.com/cello/less.js and it should be usable in some form but i also don't update it unless i need to.

now that less 1.3 has just been released, i plan to update dojo to use that but dojo doesn't use a patched version of less so that won't help you.

@cloudhead
Owner

This won't work because we need the ability to specify multiple attributes with the same name..

@neonstalwart

@cloudhead did you look closely? it allows for multiple attributes with the same name - look at the tests.

@cloudhead
Owner

Ah I just looked at the diff. If we're comparing with values too though, may as well do it for everything?

@neonstalwart

@cloudhead what do you mean? what needs to change?

@cloudhead
Owner

Ok, here's the fix: cb78933

@neonstalwart

@cloudhead i see - that does the same thing i was doing except i was avoiding calling indexOf for every rule.

i'll try to get around to opening a pull request with some more tests to cover this and another with the change to be able to run the tests from any dir.

@neonstalwart

...and i see what you mean by do it for everything - not just mixins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 21, 2011
  1. @neonstalwart

    added failing test for #49

    neonstalwart authored
  2. @neonstalwart
  3. @neonstalwart

    mixins: don't duplicate rules with the same name (fixes #49)

    neonstalwart authored
     - this maintains the behavior where all mixins that matched the call
       are applied
     - if the result of applying the mixins produces rules with the same
       name then the last rule with that name wins
     - an alternative approach would be to only apply the last mixin that
       matches the call
  4. @neonstalwart
Commits on Oct 25, 2011
  1. @neonstalwart

    mixins: duplicating rules may be intended. only remove duplicate values

    neonstalwart authored
    the following would be intentional and should be allowed:
    	.foo {
    		background: #fff;
    		background: rgba(255, 255, 255, 0.8);
    	}
This page is out of date. Refresh to see the latest.
View
13 lib/less/tree/mixin.js
@@ -8,7 +8,7 @@ tree.mixin.Call = function (elements, args, index) {
};
tree.mixin.Call.prototype = {
eval: function (env) {
- var mixins, args, rules = [], match = false;
+ var mixins, args, rules = [], match = false, cssRules = {}, l, ruleCss;
for (var i = 0; i < env.frames.length; i++) {
if ((mixins = env.frames[i].find(this.selector)).length > 0) {
@@ -25,6 +25,17 @@ tree.mixin.Call.prototype = {
}
}
if (match) {
+ // filter out rules with the same name and value.
+ l = rules.length;
+ while(l--) {
+ ruleCss = rules[l].toCSS(env);
+ if (!cssRules[ruleCss]) {
+ cssRules[ruleCss] = 1;
+ }
+ else {
+ rules.splice(l, 1);
+ }
+ }
return rules;
} else {
throw { message: 'No matching definition was found for `' +
View
18 test/css/mixins-import.css
@@ -0,0 +1,18 @@
+.a {
+ mixed: 20;
+ foo: 'bar';
+ mixed: 25;
+ main: 'yes';
+}
+.b {
+ mixed: 20;
+ foo: 'bar';
+ mixed: 25;
+ main: 'yes';
+}
+#main {
+ mixed: 20;
+ foo: 'bar';
+ mixed: 25;
+ main: 'yes';
+}
View
10 test/less-test.js
@@ -2,9 +2,7 @@ var path = require('path'),
fs = require('fs'),
sys = require('sys');
-require.paths.unshift(__dirname, path.join(__dirname, '..'));
-
-var less = require('lib/less');
+var less = require('../lib/less');
less.tree.functions.add = function (a, b) {
return new(less.tree.Dimension)(a.value + b.value);
@@ -18,13 +16,13 @@ less.tree.functions.color = function (str) {
sys.puts("\n" + stylize("LESS", 'underline') + "\n");
-fs.readdirSync('test/less').forEach(function (file) {
+fs.readdirSync(__dirname + '/less').forEach(function (file) {
if (! /\.less/.test(file)) { return }
- toCSS('test/less/' + file, function (err, less) {
+ toCSS(__dirname + '/less/' + file, function (err, less) {
var name = path.basename(file, '.less');
- fs.readFile(path.join('test/css', name) + '.css', 'utf-8', function (e, css) {
+ fs.readFile(path.join(__dirname + '/css', name) + '.css', 'utf-8', function (e, css) {
sys.print("- " + name + ": ")
if (less === css) { sys.print(stylize('OK', 'green')) }
else if (err) {
View
4 test/less/import/mixins-import-a.less
@@ -0,0 +1,4 @@
+@import "mixins-import-vars";
+.a {
+ .mixin;
+}
View
4 test/less/import/mixins-import-b.less
@@ -0,0 +1,4 @@
+@import "mixins-import-vars";
+.b {
+ .mixin;
+}
View
4 test/less/import/mixins-import-vars.less
@@ -0,0 +1,4 @@
+.mixin() {
+ mixed: 20;
+ foo: 'bar';
+}
View
11 test/less/mixins-import.less
@@ -0,0 +1,11 @@
+@import "import/mixins-import-a";
+@import "import/mixins-import-b";
+
+.mixin() {
+ mixed: 25;
+ main: 'yes';
+}
+
+#main {
+ .mixin
+}
Something went wrong with that request. Please try again.