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

Attempt to fix import sequencing #2246

Merged
merged 14 commits into from
Oct 26, 2014
Merged

Attempt to fix import sequencing #2246

merged 14 commits into from
Oct 26, 2014

Conversation

lukeapage
Copy link
Member

  • make the imports always loaded in a consistent order
  • - stretch goal process the imports containing variables after all of imports have been processed

@lukeapage
Copy link
Member Author

Not working in this pull request

main

@import 'x';
@var: "override";

x

@import 'y'+@var;
@var: "my-var"

In this situation the var does not get overridden because less see's x's scope as a scope which is different from main. Thats because the work to flatten the import into main's scope doesn't happen till after imports have been processed.
However if you remove @var: "my-var" then it will work. it will also work if var is in a file referenced from main.less.

@seven-phases-max
Copy link
Member

Yes, this looks the only way to solve this is to force @var evaluation at the point of usage (e.g. making the whole thing imperative). I was thinking of that another day and my only idea was that we could make it imperative but if the variable is redefined after it was used in @import - throw a error.
E.g x.:

@var: foo; // OK
@import "@{var}/z.less"; // force searching for latest existing @var definition at this point
@var: bar; // Error, redefinition after using in @import

@lukeapage
Copy link
Member Author

@seven-phases-max we are always in dodgy territory because
a) the variable might be redefined by a mixin that is called
b) redefined in a file in the import dependent on the variable
so we already say - only variables on their own work.
I was hoping the original restrictions would be enough to please people.. now I'm hoping this much better version is enough to satisfy people.

I was thinking how to fix it and
if you passed down with the "import" frame that it was an "import" you could combine with the variables from the parent file.. but then you don't know where the import was in that ruleset so you don't know if its an override or been over-ridden...

so we have to flatten the imports in.
So that means the only way to fix it would be to replace each import with the rules that were created for that ruleset as part of the visitor import.. I think that would work because importNodes aren't re-used until after the visitorImport (e.g. a import in a mixin would get expanded and when the mixin got called, all of the rules would get copied, minus the import node wrapper)

I should probably have a go at that here but I feel zapped this week.

What do you think about this pull-request? The first commits are quite understandable and I think make less more predictable which is good but I do worry that allowing better variable imports it just providing another way to do things in less, making it more complicated and increasing technical debt "in the field" because its better if everyone does something 1 way than a whole mix.

@seven-phases-max
Copy link
Member

What do you think about this pull-request?

I totally rely on your opinion in this case. Of course this will raise some new strange and hard-to-debug issues first, but hey they get what they want - we can always find an excuse like "Sorry guys, it's a self-recursion that is impossible to solve so use this at your own risk :)" (probably offering some more Less-friendly solutions if possible as we always do).

Of course I'll try to perform some more tests with this PR to find if it's leading to something really strange.

@lukeapage
Copy link
Member Author

anyone not ok with me pulling into beta 2? Seems appropriate to be part of v2 even if it isn't in beta 1 and being in a beta means we might find any bugs out before its released as stable

@jonschlinkert
Copy link
Contributor

I think this should be your decision. I'm 100% cool with whatever you decide.

@lukeapage
Copy link
Member Author

well.. lets go with it, Any problems should be easy to fix.

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

Successfully merging this pull request may close these issues.

3 participants