-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Fixes import by reference inlines source's inline imports - 2620 #2642
Fixes import by reference inlines source's inline imports - 2620 #2642
Conversation
present in output. Close less#2620
@@ -127,7 +127,7 @@ Import.prototype.eval = function (context) { | |||
} | |||
} | |||
|
|||
if (this.options.inline) { | |||
if (this.options.inline && !this.path.currentFileInfo.reference) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I guess the problem here is that the new node does not have isReferenced set to false on it?
And we can avoid it all because its not possible to reference an inlined file - so we just ignore rather than setting isReferenced on the imported node?
Is it worth a comment?
[edit] Sorry that sounded rude. I'm not sure if its obvious or not so if you think its obvious I'll go ahead, just questioning..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think you wrote something rude, unless you edited it away in the meantime. Suggestions like this are why I do not commit directly and send pull requests instead. It is possible for other people to review and suggest different/cleaner solution.
As I understand it, setting isReferenced
wont help. That function is used to override "imported by reference therefore do not print" int ruleset/directive/selector/comment. Basically, nobody cares about whether Anonymous
is stored in referenced file or not, it is printed
I can change the fix to produce Anonymous in eval and then do not print if Anonymous is in referenced file. I will do that later this week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure, is this about the difference in the output of the the following (artificial) example:
// ref.less
.mixin() {
@import (inline) "boo.css"
}
// master.less
@import (reference) "ref";
.mixin();
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Original use case was without mixin:
// ref.less
@import (inline) "boo.css"
// master.less
@import (reference) "ref";
The boo.css would appear in output and should not. I will add test with mixin to make sure the fix wont break that - the boo.css should appear in output in that case since it was brought in from that mixin.
node type now works the same way as visibility of ruleset or directive.
@lukeapage I amended pull request to do what you suggested. I also added the test for how it works when import inline is brought up by mixin. |
Fixes import by reference inlines source's inline imports - 2620
🎉 |
Import inline located inside file imported by reference should not be present in output. Fixes #2620