Skip to content

Common import(-once) doesn't happen after reference-import #3040

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

Closed
thybzi opened this issue Mar 21, 2017 · 13 comments
Closed

Common import(-once) doesn't happen after reference-import #3040

thybzi opened this issue Mar 21, 2017 · 13 comments
Labels

Comments

@thybzi
Copy link

thybzi commented Mar 21, 2017

One more strange and unexpected import case.

Common import (without multiple, but with expected CSS output) doesn't happen if the same file was already reference-imported earlier.

The fileset to reproduce:

main.less

@import 'foo';
@import 'bar';

bar.less

@bar-height: 42px;

.bar {
    height: @bar-height;
}

foo.less

@import (reference) 'bar';

.foo {
    padding-top: @bar-height;
}

Expected CSS output for main.less compiled:

.foo {
    padding-top: 42px;
}

.bar {
    height: 42px;
}

Actual CSS output for main.less compiled:

.foo {
    padding-top: 42px;
}
@thybzi
Copy link
Author

thybzi commented Mar 21, 2017

Currently there are two possible workarounds, but both seem to be bad:

  1. Don't reference-import bar inside foo.
    In that case, the compilation will be broken when foo is used separately.

Actually, in our code it is used separately. Just like this:

some-other-main.less

@import 'foo';
@import 'qux';
  1. Use multiple in main.less:

main.less

@import 'foo';
@import (multiple) 'bar';

But that doesn't seem to be a good practice due of several reasons:

  • The necessity of multiple-import itself isn't obvious for the case when the CSS wasn't yet output into the current scope (as reference-import doesn't output CSS).
  • The import line I should place multiple to depends on imports order.
    If I import bar and then foo, I would need to modify the line inside foo.less: @import (reference, multiple) 'bar'; (and that is even less obvious)
  • Multiple-imports may slow down the compilation significantly.
    (I just reduced LESS compilation time from 44s to 14s by removing multiple's from only two imports.)

@seven-phases-max
Copy link
Member

seven-phases-max commented Mar 21, 2017

I'm wondering if the issue should be generalized to all (applicable) import options, not just reference.
E.g.:

@import (css) "foo";
@import "foo"; // no effect despite it's even different file :)

@thybzi
Copy link
Author

thybzi commented Mar 24, 2017

@seven-phases-max Yes, I believe you're right.
I didn't test cases other than the one I describe, but I give you a carte blanche to rename/generalize this issue.
Thank you.

@thybzi
Copy link
Author

thybzi commented Mar 27, 2017

Just to clarify my use-case.

In our projects there are several css builds (say, final css files) that may include different combinations of common less files (components).

Some components use variables and mixins from other components, so if they go to some build that doesn't include them, LESS compilation will fail.

So I'm trying to organize requirejs-like dependency structure, putting reference imports on top of the dependent component code.

@seven-phases-max
Copy link
Member

Mixins and variables should not require reference imports. (If they do something is even more wrong).

@thybzi
Copy link
Author

thybzi commented Mar 27, 2017

Mixins and variables should not require reference imports. (If they do something is even more wrong).

That is true if file doesn't contain anything but mixins and variables.
Actually, that may also contain styles :)

One more case is extend'ing "donor" selector without outputting it.

@seven-phases-max
Copy link
Member

One more case is extend'ing "donor" selector without outputting it.

This hack (used only to emulate non-existing "extending mixins" feature) is better to be hidden in a separate file, and no top-level Less component should still ever use reference.

@thybzi
Copy link
Author

thybzi commented Mar 27, 2017

So, generally you're saying "We have reference import, but don't use it. Put your globals to separate file instead" :)

@seven-phases-max
Copy link
Member

seven-phases-max commented Mar 27, 2017

The reference import is invented as a kludge to re-use existing CSS libraries only, no Less library is supposed to ever have it inside in a normal situation (it's OK to have it for the extend hack but the hack shall be isolated in a separate file so that when it's no longer needed you don't have to remove it from everywhere).

@thybzi
Copy link
Author

thybzi commented Mar 27, 2017

A frightening thing you're saying! :-O
I think this somehow should be mentioned in the doc: http://lesscss.org/features/#import-options
Because when I first found this, I said to myself: "Cool feature, now we can put @input-height on top of input.less and import only variable but not styles — without need for a separate file`.

And some time ago I also proposed a similar story to Stylus: stylus/stylus#2139

BTW, how a CSS library may be used with reference (if not for extend, uh)? Maybe you're speaking about inline or css option?

@seven-phases-max
Copy link
Member

seven-phases-max commented Mar 27, 2017

That is, if for example you have something like extend .clearfix with the said "reference hack" in a Less library it's ideally to be made like this:

// "clearfix.less":
@import (reference) "clearfix-auxiliary-details.less"; // only in this file and nowhere else
.clearfix_() {
    &:extend(.clearfix_ all);
}

So anywhere else in you library you always import "clearfix.less".
Notice the reusable .clearfix_ mixin (and .clearfix_ class defined within "clearfix-auxiliary-details.less") should not be mixed or mixed-up with any to-be-used-in-html CSS .clearfix class (even if it's essentially the same thing).

@seven-phases-max
Copy link
Member

seven-phases-max commented Mar 27, 2017

I think this somehow should be mentioned in the doc:

Well, not everybody will be agree with me (specifically in calling it "kludge"). There's a ticked where we were discussing this in details (1: "it's a hack, we should advertise to minimize or at least limit its use" -> 2: "yeah maybe it'a hack, but it's a cool hack" -> 1: "...." etc.) but I can't find it right now.

Well, just take the count into account: "extend/reference" issues/bugs open by now (it's tens, not just a few). This is because when reference and extend were proposed and implemented, Less files and Less code were considered just "a-sort-of", i.e. in practice nothing but very simple and flat one-level-deep imports/css-like-only-code examples were taken into account.

P.S. As for your example at tylus/stylus#2139, it's fine as long as we do not enter another "thing to avoid" when designing a library from scratch (as oppose to easy re-use of existing libraries): "never reuse existing CSS rulesets as mixins and/or with extend" (it's another big story though and I do not want to pollute the ticket even more...).

@stale
Copy link

stale bot commented Nov 14, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

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

No branches or pull requests

2 participants