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

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

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

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

vicb opened this issue Jun 25, 2010 · 49 comments

Comments

@vicb
Copy link
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

@cloudhead
Copy link
Member

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

@jamesfoster
Copy link
Contributor

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

@cloudhead
Copy link
Member

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

@jamesfoster
Copy link
Contributor

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.

@cloudhead
Copy link
Member

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.

@vicb
Copy link
Contributor Author

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.

@jamesfoster
Copy link
Contributor

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.

@normanzb
Copy link

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

@NielsJanssen
Copy link

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.

@divyanshu
Copy link

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

@ianstormtaylor
Copy link

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

@honi
Copy link

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.

@ianstormtaylor
Copy link

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

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.

@ianstormtaylor
Copy link

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.

@wlangstroth
Copy link

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

@neonstalwart
Copy link
Contributor

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.

@wlangstroth
Copy link

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

@neonstalwart
Copy link
Contributor

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

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

geddski commented Nov 3, 2011

having same issue

@dbergey
Copy link
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.

@neonstalwart
Copy link
Contributor

fyi #431 is a pull request that fixes this issue

@Kalyse
Copy link

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.

@leeight
Copy link
Contributor

leeight commented Mar 21, 2012

The same issue :-(

@Kalyse
Copy link

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: #49

@neonstalwart
Copy link
Contributor

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

@cloudhead
Copy link
Member

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

@cloudhead
Copy link
Member

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.

@sashasklar
Copy link
Contributor

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

@jeremyricketts
Copy link

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

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.

@jeremyricketts
Copy link

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

geddski commented Mar 21, 2012

@jeremyricketts good point.

@nicholashead
Copy link

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.

@leeight
Copy link
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;
}

@jreading
Copy link
Contributor

+1 jeremyricketts

@neonstalwart
Copy link
Contributor

anybody want to step up - https://twitter.com/#!/neonstalwart/status/183247994072735744

@jeremyricketts
Copy link

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

@sashasklar
Copy link
Contributor

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:
#49 (comment)

@Kalyse
Copy link

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

Kalyse commented Mar 26, 2012

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

@cloudhead
Copy link
Member

Here's the fix: cb78933

@cloudhead
Copy link
Member

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

Cheers

@matthew-dean
Copy link
Member

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

@basher
Copy link

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

@jreading
Copy link
Contributor

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

@bgw
Copy link

bgw commented Jun 3, 2012

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

@lukeapage
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests