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

Variables in import statements. #2772

Closed
Giwayume opened this issue Jan 12, 2016 · 32 comments
Closed

Variables in import statements. #2772

Giwayume opened this issue Jan 12, 2016 · 32 comments

Comments

@Giwayume
Copy link

I get "SyntaxError: variable @url is undefined" with the following logic:

.dynamic-import(@url) {
    @import "@{url}";
}
.dynamic-import("test/definition.less");

It appears imports are not as dynamic as I'd hoped. Is this expected behavior?

Funny thing is if I don't even call the mixin (only declare it), there's no errors.

.dynamic-import(@url) {
    @import "@{url}";
}

Note: tested on 2.7.3

@seven-phases-max
Copy link
Member

Yes, this is known problem. Imports have to be evaluated before mixins (or anything else at all) - read comments there for more details (and see why it's better to avoid such code even if the issue is resolved somehow).
Also notice that even if it will work - none of variables defined in "test/definition.less" can override any of variables defined at the scope of .dynamic-import call (thus .dynamic-import(@url) can never become equal to @import "@{url}" - thus potentially throwing more unexpected problems at you - thus there most likely be not too much interest in making your example to work ever).

@seven-phases-max
Copy link
Member

(I would probably leave this a open feature-request, but I doubt it makes sense while knowing it's barely to be ever implemented, so I leave it to you).

@seven-phases-max
Copy link
Member

I added less/less-docs#388 (trying to describe it in a user-friendly words...).

@Giwayume
Copy link
Author

I guess I'll attempt to explain my real problem here and maybe there is an existing solution. I have a framework of less files and a couple of themes (sets of variables) sitting on a server.

There are multiple applications that need to reference this common framework, and these applications pick & choose a theme. The problem comes with the pick & choose part; I need a way to allow each application to pick a theme, but at the same time if it doesn't specify one a default theme will be used.

So an example application.less would be:

@import "/path/to/framework.less";

If an application specifies a theme, my initial thought was:

@import "/path/to/framework.less";
@theme: "theme-name";

The problem is I can't figure out the "default" part. It's not working with variables (my example in #2771) and I can't use mixins hence this issue.

@seven-phases-max
Copy link
Member

Can't you simply move theme import into the app itself? E.g.:

// in either proper order:
@import "/path/to/framework-theme.less";
@import "/path/to/framework.less";

@Giwayume
Copy link
Author

The problem I'm trying to solve is having a default theme so it's not required to specify a theme. This means it needs to be a variable somewhere, and apparently the 'solution' less has for default variables doesn't work across imports?

@Giwayume
Copy link
Author

I'll move this here.
Expected: @import "desired/path";
Actual: @import "default/path";

application.less

@import "framework.less";
@myPath "desired/path";

framework.less

@myPath "default/path";
@import "@{myPath}";

@seven-phases-max
Copy link
Member

Apparently the 'solution' less has for default variables doesn't work across imports?

No, it is just variable usage inside @import statements is limited.
In any other statements variable overriding works across different files as expected.
(For comparison notice that in Sass you can't use variables in @imports at all ).

@seven-phases-max
Copy link
Member

Btw., (I'm just keeping to offer workarounds) also notice that if you remove @my-path from framework.less it will work (if I'm not mistaken - can't test it right now). Obviously this means that you still have to require it to always appear in app.less but:

// in either order
@import "framework.less";
@theme "default";

is probably better than nothing?

@Giwayume
Copy link
Author

Yes, I noticed that too but it doesn't solve the problem haha. It's also really weird that it works like that.

If I can't specify a default then there is no point in having external configuration in this case. You have no ideas for handling default variables mixed with import statements?

@matthew-dean
Copy link
Member

Are the imports a "known" and limited list? If so, you could hardcode the imports, but actually "include" them via mixins guarded with variables.

@Giwayume
Copy link
Author

No, we often create new themes when new applications are created, but not all the time. This type of syntax is necessary.

@import "@{theme-folder}/@{theme}";

@rjgotten
Copy link
Contributor

What about:

@import "/path/to/framework.less";
@import "/path/to/base-theme.less";
@import (optional) "@{optional-theme}"

Where base-theme.less defines @optional-theme as a non-existant file, and then allow overriding that with a variable injection done via the compiler commandline?

@seven-phases-max
Copy link
Member

I'd rather use framework-default-theme.less (written as:

@theme "default";
@import "framework.less";

) for default-themed apps. And then framework.less to be imported only with custom theme apps.

@Giwayume
Copy link
Author

Gah I'm scratching my head can't get this to work. I'm trying to simplify it for explanation but it's not that simple.

For example, each of my UI components are scoped so I can use the same theme variables across components. E.g.

& { @import "definitions/grid.less"; }

Then each component (say grid.less) imports theme.less, which imports the default theme variables then overrides with the selected theme's variables.

@import "@{theme-folder}/default/@{element}.variables";
@import (optional) "@{theme-folder}/@{theme}/@{element}.variables";

@theme is also set per component, so it's not just one global theme variable. It's a problem, say, I have 50 components and I can't set defaults for the consumer of the framework.

Since my components are scoped, I either need a mixin I can call in every scope or I need a variable to dynamically reference a theme.config file that sets the theme variables in each component scope. But as I've seen here:

  1. Imports happen before mixins so you just get undefined variables if you rely on a mixin to set a variable for an import
  2. The user can't override a @theme-config-path variable of sorts because imports take the first variable they find for some reason

@Giwayume
Copy link
Author

By the way @theme-config-path would be relative to the component's less file and not the framework consumer's less file which is a reason I don't like option 2.

@seven-phases-max
Copy link
Member

Hmm, I've just realized you probably try to copy Semantic-UI customization approach (or is it Semantic-UI itself?), do you?

Imports happen before mixins so you just get undefined variables if you rely on a mixin to set a variable for an import

Imports happen before anything, incl. variables (thus the compiler cannot evaluate variables before it finishes all imports and in the same time it can't apply the imports that try to use the said variables. So technically, in a "strict lazy-evaluation" context, variables should not be allowed in imports at all, and while Less provides the feature, it still is nothing but a back-door so it's important to keep in mind that using it as the core of a library customization approach is ...).

That being said, while both "file-based overriding" and "variable-based overriding" work just fine on their own, using the latter to override the former is expected to open that can of worms you face. I don't think there're can be any solution (and even if you find a workaround for the particular problem you'll face more as things go further...)
That's why in first place there I made a remark that the whole approach is somewhat anti-Lessish
(that is, "file-based overriding" works in Less just fine, but if it's to be used it's better to be designed to use the method they typically use in environments/languages where such practice is a mainstream - imports/includes are customized via "directory priorities" and/or macros (not counting that Less has no macros of course)).

@Giwayume
Copy link
Author

Yes I look a lot of "inspiration" from semantic-ui =)

The problem with the way they have it setup is the entire library is expected to be copied into every project. I'm trying to create shared resources.

@seven-phases-max
Copy link
Member

I'm trying to create shared resources.

Then you'd really to consider some alternatives (because as you can see in the ticket linked above, initially the SUI overriding approach is designed w/o even considering the base Less principles. This of course does not automatically imply the method is the worst one, but still it's something to keep in mind).

@Giwayume
Copy link
Author

I get the methodology and what less is trying to be. However, if you're already breaking the methodology for developer convenience by having variables inside of import strings, maybe try to make it not so hacky.

It seems the actual parser process is similar to this:

  1. Scrape all the variables in the current file, last ones win.
  2. Run imports based on the "winning" variables.
  3. Scrape all variables in embedded imported files, these new variables override any existing "winning" variables from the parent file that initiated the import.
  4. Run imports again, go to step 3.

Or this:

  1. Scrape all the variables up to the import statement.
  2. Defer import if not all needed variables encountered.
  3. Resolve the rest of the variables in file.
  4. Run deferred imports. If variable never encountered, throw error.

Whereas this would allow default variables across imports:

  1. Scrape all the variables in the current file, last ones win.
  2. Run imports based on the "winning" variables.
  3. Scrape all variables in embedded imported files, but keep variable presidence based on whether variables occurred before or after the import.
  4. Run imports again, go to step 3.

@seven-phases-max
Copy link
Member

Cycled variables after import after variables after import after variables .. and so on evaluation does not look like something "not so hacky" for the implementation itself.
Personally I do not mind if the behaviour is improved, so won't mind if any improving PR comes in.
(But to be honest our discussion made me to realize I should start to actively advertise against such approach and frameworks using it. As the more popular those get the more "impossible" feature-requests will pile up).

@seven-phases-max seven-phases-max changed the title Dynamic importing inside of a mixin. Variables in import statements. Jan 23, 2016
@SomMeri
Copy link
Member

SomMeri commented Jan 23, 2016

Would something bad happen if import variables would work the same way as mixin variables? Variables used in import statement could be treated the same way as variables in mixin arguments and variables defined by import could be treated as variables returned from mixins.

@Giwayume
Copy link
Author

My main concern (as a user of less) is that the target implementation behaviour isn't so hacky, not the parsing method. I was guessing at why the current behavior is happening:

main.less

@import "imported.less";
@url: "overrided.html";

imported.less

@url: "default.html";
@import "@{url}";

Today the second file imports "default.html", however if you remove the @url: "default.html"; in imported.less, it imports "overrided.html" instead. This is the hacky behavior I'm talking about. I don't know how to describe whatever parser logic causes this strange behavior besides the two guesses I made above.

The desired behavior would be in either case, whether @url: "default.html"; was removed or not, imported.less would import "overrided.html" because it was the last instance of that variable encountered in both files.

@matthew-dean
Copy link
Member

@SomMeri @seven-phases-max I feel like this is just another variation of the mixin-related "variable visibility problem". That is, a circular evaluation dependency. Variables in imports are extremely useful, but imports import variables into the main scope, which can include variables which, when evaluated, change the way the variable'd import statement is evaluated.

As circular dependency chains are part of the a fundamental design of Less.js, there's not necessarily "one way" to approach the problem.

However, in practice, circular evaluation of a variable in an import shouldn't take that many "loops". So @Giwayume, to your point, I would think your first description of what the parser should do (and your follow-up example) is the correct one.

That is, in short, a stylesheet should call imports based on the "final" variable values in the current scope, and, if necessary, call the @import again if the value of that var changes when evaluating. While a slightly different case, Less already has to do this with mixins.

@seven-phases-max I don't know why we would recommend against this use case, when the rest of Less follow this same model of needing to sometimes evaluate statements more than once.

@rjgotten
Copy link
Contributor

However, in practice, circular evaluation of a variable in an import shouldn't take that many "loops".

Sorry, but no: when scenarios such as @import "@{url}" redefining the url variable are possible and end up triggering the grandfather paradox, you are dealing with a problem that is fundamentally unsolvable via iteration.

@matthew-dean
Copy link
Member

@rjgotten Of course it's not unsolvable. It's just undefined behavior. As @Giwayume pointed out, there are a few possible solutions to such paradoxes. You could equally call a variable in an import unsolvable, since evaluation happens after importing. But that was more or less solved.

@rjgotten
Copy link
Contributor

@matthew-dean
I said unsolvable via iteration, not unsolvable in general.

Ofcourse you can still solve it: you have to agree on a convention that cuts open the loop of the cyclic dependency. ;-)

@SomMeri
Copy link
Member

SomMeri commented Jan 26, 2016

@matthew-dean You mean it should call the same import statement twice when something changes in variables or rather that call imports sequentially one by one where later imports reflect previous variables?

@matthew-dean
Copy link
Member

@SomMeri I did say that... but not necessarily. I understand why @Giwayume's example fails, but there could be a tweak that would make it work.

Currently, we have to initially evaluate vars (as best we can) in order to decipher this at all (part 1):

@import "@{url}";

Originally, this was only available in the root document. Later, a fix was added so that you could evaluate vars in @import statements within imports themselves (part 2).

So, in this particular example given by @Giwayume.

main.less

@import "imported.less";
@url: "overrided.html";

imported.less

@url: "default.html";
@import "@{url}";

My guess is this fails because there's only an immediate evaluation (within imported.less) of this variable.

So, if we are doing a partial evaluation of root, and a partial evaluation of the import, it seems like we should be able to override @url from imported.less with @url from main.less in order to resolve the @import correctly.

It's not an easy problem, and I agree with @seven-phases-max at least in part that this problem doesn't have to exist at all if library authors weren't taking this approach.

At the same time, I think it's reasonable to call the example above a bug, or, if not, unexpected behavior based on what we currently allow as far as evaluating vars in imports.

As to: should we evaluate an import and then re-import it if a var changes (and then discard the original import in final evaluation)? On the one hand, that might be useful. On the other hand, it seems like a low priority. Thinking about it some more, that's probably a bit overkill and might add too much evaluation overhead for too small a gain.

So, I think the only tweak here as far as evaluation isn't that circular. It just requires the parent scope to have it's vars partially evaluated (I say partially, since it wouldn't be an evaluation of other mixins / imports into the parent scope -- this should only be a list of @var: value declarations), as a lookup list for imports within that parent.

I've been thinking of patterns where it might be possible for Less to do a kind of "continuous evaluation" rather than the more linear evaluation that happens now, but that's outside of this cope.

@SomMeri
Copy link
Member

SomMeri commented Jan 26, 2016

@matthew-dean Then it can work the same way as mixins now - second mixin in a row sees variables returned from the first one. Since it was implemented already once, it should be easier to do the same thing again :).

This works:

.mixin() {
  use: @variable;
}

.import-variable() {
  @variable: value;
}

#use-place {
  .import-variable();
  .mixin();
}

This works too:

.import-variable() {
  @variable: value;
}

.mixin(@variable) {
  use: @variable;
}

#use-place {
  .import-variable();
  .mixin(@variable);
}

both compile to:

#use-place {
  use: value;
}

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

@stale stale bot added the stale label Nov 14, 2017
@stale stale bot closed this as completed Nov 28, 2017
@jflayhart
Copy link

jflayhart commented Mar 13, 2018

No, it is just variable usage inside @import statements is limited.

It's just that this has worked fine up to version 2.7.3, at least that I can tell. Upgrading to v3 does not allow variables be used within imports across separate files. So it seems if this type of behavior was not expected to work, it certainly used to (although I am not a LESS guy, so maybe I'm missing something as to how it used to work).

This is Semantic UI's fault! haha 👎 If using Semantic UI + Webpack for this, DO NOT upgrade past version 2.7.3 until either Semantic improves their LESS best practices or LESS gets to a point where they can essentially handle dynamic imports with variables across separate files.

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

7 participants
@matthew-dean @rjgotten @SomMeri @Giwayume @seven-phases-max @jflayhart and others