duplicated properties when @importing multiple times (nested @import) #49

Closed
vicb opened this Issue Jun 25, 2010 · 49 comments

Comments

Projects
None yet
Contributor

vicb commented Jun 25, 2010

Master @imports A & B,
B @imports A

When using a mixin from A in the master file, the properties get duplicated

See the unit test for more info

Config: lessc cli version 1.0.21 on ubuntu 10.04

Owner

cloudhead commented Jun 26, 2010

I guess the issue is that the file gets imported twice, that's the correct behaviour for mixins.

Contributor

jamesfoster commented Jun 26, 2010

Do you think it should import the file twice tho? If not it should also take into account relative urls. For example

// in main.less
@import: imports/import1.less;
@import: imports/import2.less;

// in imports/import1.less
@import: import2.less; // don't import this
@import: imports/import2.less; // do import this
Owner

cloudhead commented Jun 27, 2010

hmmm, I wonder if there's an easier way to check if a file is equal to another file. Maybe something in the headers.

Contributor

jamesfoster commented Jun 27, 2010

File size is a cheap indicator and is most likely unique in any given set of source files. However it is possible to have 2 files with the same size.

Similarly with date modified.

If the server sets the ETag header you can / should use that.

We use the MD5 hash of the file contents as the key when caching but I personally think that's overkill.

Owner

cloudhead commented Jun 27, 2010

hmmm, yea the problem is javascript doesn't have native MD5, or I would use that..
ETag would be ideal, but some servers don't set it. I'll have to think about this.

Contributor

vicb commented Jun 28, 2010

It seems that we agree that the files should not be imported twice. However is it always true ? (i.e. with the help of scope, importing in a mixim might be ok - cloudhead you can answer better than me on this one).

Many languages have already solved this issue in different ways:

  • C -> #ifndef / #define / #endif
  • PHP -> include_once()

The "C way" might be a bit harder to implement but conditionals might offer other great possibilities.

If the "PHP way" is chosen we need to choose a way to differentiate files. The absolute URL seems a good pick (we have it either directly or via document.location + relative URL) - I think it's better than size / length / MD5 because it does not consumes an HTTP request.
However it might not be enough: each URL maps to a single file but each file might be mapped to several URLs. In such case introducing a new keyword @@name: <unique name> might help.

The @import_once algo would then be: if the absolute URL has not already been imported, get the files anf check if the @@name as already been imported, if not import the file.

Contributor

jamesfoster commented Jun 29, 2010

I agree. within the context of a single request you should be able to assume that the resources dont change. so the absolute urls should be good enough.

checking for file changes is another matter.

i think we should keep @import import multiple times because this is how original @import works,

i like @vicb's php-like solution, but instead of @import_once i like something better in 'comment' so it still compatible with original css syntax.

can we add something like this on the top of the .less file:
/!require:url(./abc.less)/

I am experiencing this problem as well. My project has a hierarchy of LESS files compiling into a single .css file. There is one utility LESS file which is included in several files, in the end result all mixins are duplicated the same amount of times as that utility file has been imported.

An @import_once, or perhaps @import: once url('url'); would solve this problem.

We are facing the same issue as @NielsJanssen on our project, any idea by when this issue will be fixed??

Also running into this issue. Anyone figure out a solution?

honi commented Sep 23, 2011

Having the same issue here. Couldn't find a simple solution, just ended up re arranging the imports so that they don't get imported twice.

I'd really suggest checking out Stylus if you use node.js. I used LESS for a while, got frustrated with its complete lack of development, switched to SASS and then finally to Stylus. It really nails the features, the syntax is optional (and I use a middle-ground), its very powerful, and TJ is a really responsive developer of it.

If you don't use node.js, you can still use Stylus with ruby and compiling on your machine. And if you don't like Stylus for any reason, SASS/SCSS is also a nice alternative and can be done the same way.

Long story short: LESS is no good in the long run.

honi commented Sep 23, 2011

Very bad attitude man.

It's not necessary to post that bs in here. You could have sent it by mail or alike. You can't have such high standards like wanting a "really responsive developer" for something that is free to use.

Absolutely not.

I would have loved that advice when I first found LESS so that I didn't waste weeks of my time getting used to it, and converting to it and from it when I eventually left. Granted I should have noticed it myself first, as there are more open issues than closed ones, and most of them haven't been responded to.

It's not about what standards I can or can't have. It's about providing a warning to people, when there are better alternatives.

So I stand by my "BS" and hope people find the warning helpful.

@ianstormtaylor: saying a project is "no good in the long run" is a bit hysterical.

Contributor

neonstalwart commented Oct 20, 2011

having this same problem too.

foo.less

@import "bar.less";
@import "baz.less";

bar.less

@import "mixins.less";
.bar {
  .mixer
}

baz.less

@import "mixins.less";
.baz {
  .mixer
}

mixins.less

.mixer {
  color: 000;
  border: 2px solid #fff;
}
$ lessc foo.less
.mixer {
  color: 000;
  border: 2px solid #fff;
}
.bar {
  color: 000;
  border: 2px solid #fff;
  color: 000;
  border: 2px solid #fff;
}
.mixer {
  color: 000;
  border: 2px solid #fff;
}
.baz {
  color: 000;
  border: 2px solid #fff;
  color: 000;
  border: 2px solid #fff;
}

i stayed out of this discussion earlier but now that i'm up against this same issue... @wlangstroth i don't think @ianstormtaylor is being at all hysterical. just check the list of open issues for this project and how long some of them have been opened. less is a useful tool but i think its a fair assessment that support is limited and that gets to be very frustrating while waiting.

i get the impression that @cloudhead mostly ignores any comments i make (maybe i'm wrong about this) but it would be better to hear "i'm busy" or "i don't want to fix that" or even something harsher rather than get no response at all.

You could just have a main.less file that contained all of your imports. See the twitter bootstrap for an example (the main file is bootstrap.less).

Contributor

neonstalwart commented Oct 21, 2011

some of the less files i import (from an external library) are written to be able to be compiled stand-alone so they each include a common variables.less and the problem i'm seeing occurs because i import each of those less files into one main file and then compiling that file applies each mixin as many times as the mixins get included (once for every file i include from the external library).

you're right - i could do something like you suggested and i am doing something like that in the code i have complete control over but that doesn't mean everybody does it like that.

also, a workaround (only if i wasn't using a 3rd party library) doesn't change that this is a bug. i've become quite familiar with how to workaround issues with less but its frustrating that bugs like this have been open for almost 18 months. (@wlangstroth i realize that's not your fault)

@neonstalwart neonstalwart added a commit to neonstalwart/less.js that referenced this issue Oct 21, 2011

@neonstalwart neonstalwart added failing test for #49 ce5a810
Contributor

neonstalwart commented Oct 21, 2011

for anyone who is interested, i've got a brute force fix (not tested against @vicb's test but should work) that i pasted in a comment on #431. i'm going to try and find a better solution if i get some more time.

@neonstalwart neonstalwart added a commit to neonstalwart/less.js that referenced this issue Oct 21, 2011

@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 neonstalwart added a commit to cello/less.js that referenced this issue Oct 24, 2011

@neonstalwart neonstalwart added failing test for #49 ff6b7ff

@neonstalwart neonstalwart added a commit to cello/less.js that referenced this issue Oct 24, 2011

@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
58d6769

geddski commented Nov 3, 2011

having same issue

@csnover csnover added a commit to csnover/less.js that referenced this issue Jan 12, 2012

@csnover csnover Implement neonstalwart's fix for issue #49 from neonstalwart/1bee790a.
Note that this causes some mixin tests to fail because it deduplicates rules instead of deduplicating the actual mixin calls.
df04471
Contributor

dbergey commented Feb 3, 2012

Also had this problem, but fixed it by importing my mixin libraries into my bootstrap.less. Didn't realize subsequent imports would have access to them, but it makes sense.

Contributor

neonstalwart commented Feb 3, 2012

fyi #431 is a pull request that fixes this issue

Kalyse commented Mar 14, 2012

@cloudhead Would you be able to apply a fix for this. This is probably one of the oldest issues that is still open. Would be nice to see it resolved.

Contributor

leeight commented Mar 21, 2012

The same issue :-(

Kalyse commented Mar 21, 2012

As a suggestion to anyone who is having troubles with this issue, I would recommend that you drop Alexis a message on Twitter. Alexis has previously said in several tickets that he can't monitor all of the issues and really only fixes when he is made aware of the severity. I regard this is a severe issue but haven't been able to bring it to Alexis' attention on Twitter.

Perhaps someone else might have more luck.

Twitter: https://twitter.com/#!/cloudhead

And drop a link in to this issue: cloudhead#49

Contributor

neonstalwart commented Mar 21, 2012

@Kalyse if @cloudhead can't adequately monitor the issues of this project then that is even more reason for everyone to avoid using it. people have suggested that he should nominate some other people to help with managing the backlog of issues but again there was no response.

why should i need to use twitter to get someone's attention when they can already get alerts when i mention them in an issue? i think its shameful that @cloudhead can't manage to respond to issues that have been open for 2 years - he could at least find a couple of people he trusts and have them start working through the backlog of open issues. they could at least close duplicates and help reduce the number of open issues for him.

Owner

cloudhead commented Mar 21, 2012

The github notification system is completely useless - I get about 70-100 notifications per day, so I prefer to just ignore them.

I'm gonna look into this..

Owner

cloudhead commented Mar 21, 2012

Ok, I added an @import-once directive - it's pretty rudimentary, as it only checks that the paths match - but it should cover most use-cases.

Contributor

sashasklar commented Mar 21, 2012

@cloudhead Glad you got around to addressing this issue! Why not filter duplicate rules in the output?

I personally don't understand why this project is even on Github if pull requests aren't even considered or why the issue tracker is even running if issues go ignored.

geddski commented Mar 21, 2012

Easy folks! If any one person had a project this popular they'd be in the same boat. @cloudhead has done a great job and maybe does need to add a few trusted people as admins. But issues with pull requests and tests are much more helpful than issues alone. Maybe fix some issues while you're chilling out.

People have fixed plenty of issues, sometimes years ago. Go check out one of the 74 outstanding pull requests with no response. As an example, this very issue has many dupes going back 2 years (like #324 #71). Here's a pull request that would have fixed this issue pretty simply: https://github.com/cloudhead/less.js/pull/431The commiter asked for feedback, was met with silence, and then eventually gave up.

@cloudhead - Alexis, this is just too great a project to let it fall into disrepair. When people see the kind of thing mentioned above, it leaves them with the impression that the project is untrustworthy or unreliable. And it's unnecessary! The magic of github is that you can easily find some people who are writing great code and interested in committing to the project. Ask those good people if they will help moderate issues and pull requests.

We all love your work. The community wants to help. Let us help!

geddski commented Mar 21, 2012

@jeremyricketts good point.

I agree with @jeremyricketts -- at a company I work for, we ended up not going with LESS (went the SASS route) because of the lack of updates/bug fixes in this repo.

Contributor

leeight commented Mar 22, 2012

@cloudhead it looks like the @import-once directive does not work, this is my test case.

// a.less
.gain-bfc() {
  overflow: hidden;
  *zoom: 1;
}

// b.less
@import-once "a.less";

// c.less
@import-once "a.less";
@import-once "b.less";

div {
  .gain-bfc();
}

after compile the c.less, the expected result should be

div {
  overflow: hidden;
  *zoom: 1;
}

but i got the duplicated properties

div {
  overflow: hidden;
  *zoom: 1;
  overflow: hidden;
  *zoom: 1;
}
Contributor

jreading commented Mar 23, 2012

+1 jeremyricketts

Someone with some actual programming chops is needed. My world is PSD's, pencil and paper, and html CSS and light jQuery work.

Contributor

sashasklar commented Mar 24, 2012

A couple people are needed just to triage the issues and pull requests,
prune duplicates, make sure there are test cases for bugs, etc. I'd like
to volunteer to help with that at the very least, and I can probably help
close the smaller bugs too.
On Mar 23, 2012 1:28 PM, "Jeremy Ricketts" <
reply@reply.github.com>
wrote:

Someone with some actual programming chops is needed. My world is PSD's,
pencil and paper, and html CSS and light jQuery work.


Reply to this email directly or view it on GitHub:
cloudhead#49 (comment)

Kalyse commented Mar 26, 2012

@cloudhead Just in case you are struggling to think of a good fix for this, you should take a look at @neonstalwart resolution from a while ago.

Basically, rules should never be added to selectors if there is an existing rule with the same value as the current property. You have to check for property value as well, since with backgrounds, you might add several different backgrounds since they are handled differently in different browsers.

What do you think of this solution @cloudhead
Seems like the way forward?

Not fixing this means:

  1. Files are larger than they need to be.
  2. Spreading your CSS accross multiple files and importing lots becomes undesirable because for each time you include an additional file which also includes the mixins, you are adding that mixin's values again. I have maybe 80 lessCSS styles, and this means that when I have a doing a .background() gradient mixing, results in 80 * 6 additional styles for each selector. (6 to support all the different browsers).
  3. It also slows down page rendering. My draws/refreshes per second drops dramatically because of the additional styles.

Thoughts @cloudhead ?
Cheers.

Kalyse commented Mar 26, 2012

@cloudhead If we make a pull request for this issue from the latest commit, will you look at merging?

Owner

cloudhead commented Mar 27, 2012

Here's the fix: cb78933

Owner

cloudhead commented Mar 27, 2012

@Kalyse can you drop me an email? alexis@cloudhead.io

Cheers

Owner

matthew-dean commented Apr 26, 2012

Maybe what's needed are some additional trusted developers who can approve pull requests?

basher commented Apr 27, 2012

@cloudhead I use WinLess to compile my LESS code... WinLess comes with latest distro of less.js (see marklagendijk/WinLess#14), so any idea when this (and other fixes) will be added to latest release?

Thanks (for a great product).

Contributor

jreading commented May 10, 2012

so, er uh... How we doing on this?

bgw commented Jun 3, 2012

@jreading I think it has been fixed in git with commit cb7893

Owner

lukeapage commented Jul 17, 2012

Appears fixed (or at least original issue is) and if not, I'm sure there is another bug to cover this.

lukeapage closed this Jul 17, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment