Skip to content

Commit

Permalink
Add allowRoot property to nodes that can be returned by a function in…
Browse files Browse the repository at this point in the history 
…to root or ruleset
  • Loading branch information
@matthew-dean
matthew-dean committed Feb 2, 2016
1 parent a58bb76 commit b67403d
Show file tree
Hide file tree
Showing 11 changed files with 13 additions and 10 deletions.
1 change: 1 addition & 0 deletions lib/less/tree/comment.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ var Comment = function (value, isLineComment, index, currentFileInfo) {
this.value = value;
this.isLineComment = isLineComment;
this.currentFileInfo = currentFileInfo;
this.allowRoot = true;
};
Comment.prototype = new Node();
Comment.prototype.type = "Comment";
Expand Down
1 change: 1 addition & 0 deletions lib/less/tree/directive.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ var Directive = function (name, value, rules, index, currentFileInfo, debugInfo,
this.debugInfo = debugInfo;
this.isRooted = isRooted || false;
this.copyVisibilityInfo(visibilityInfo);
this.allowRoot = true;
};

Directive.prototype = new Node();
Expand Down
3 changes: 2 additions & 1 deletion lib/less/tree/extend.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ var Extend = function Extend(selector, option, index, currentFileInfo, visibilit
this.parent_ids = [this.object_id];
this.currentFileInfo = currentFileInfo || {};
this.copyVisibilityInfo(visibilityInfo);

this.allowRoot = true;

switch(option) {
case "all":
this.allowBefore = true;
Expand Down
1 change: 1 addition & 0 deletions lib/less/tree/import.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ var Import = function (path, features, options, index, currentFileInfo, visibili
this.path = path;
this.features = features;
this.currentFileInfo = currentFileInfo;
this.allowRoot = true;

if (this.options.less !== undefined || this.options.inline) {
this.css = !this.options.less || this.options.inline;
Expand Down
1 change: 1 addition & 0 deletions lib/less/tree/media.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ var Media = function (value, features, index, currentFileInfo, visibilityInfo) {
this.rules = [new Ruleset(selectors, value)];
this.rules[0].allowImports = true;
this.copyVisibilityInfo(visibilityInfo);
this.allowRoot = true;
};
Media.prototype = new Directive();
Media.prototype.type = "Media";
Expand Down
1 change: 1 addition & 0 deletions lib/less/tree/mixin-call.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ var MixinCall = function (elements, args, index, currentFileInfo, important) {
this.index = index;
this.currentFileInfo = currentFileInfo;
this.important = important;
this.allowRoot = true;
};
MixinCall.prototype = new Node();
MixinCall.prototype.type = "MixinCall";
Expand Down
1 change: 1 addition & 0 deletions lib/less/tree/mixin-definition.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ var Definition = function (name, params, rules, condition, variadic, frames, vis
this.optionalParameters = optionalParameters;
this.frames = frames;
this.copyVisibilityInfo(visibilityInfo);
this.allowRoot = true;
};
Definition.prototype = new Ruleset();
Definition.prototype.type = "MixinDefinition";
Expand Down
1 change: 1 addition & 0 deletions lib/less/tree/rule.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ var Rule = function (name, value, important, merge, index, currentFileInfo, inli
this.inline = inline || false;
this.variable = (variable !== undefined) ? variable
: (name.charAt && (name.charAt(0) === '@'));
this.allowRoot = true;
};

function evalName(context, name) {
Expand Down
1 change: 1 addition & 0 deletions lib/less/tree/ruleset-call.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ var Node = require("./node"),

var RulesetCall = function (variable) {
this.variable = variable;
this.allowRoot = true;
};
RulesetCall.prototype = new Node();
RulesetCall.prototype.type = "RulesetCall";
Expand Down
1 change: 1 addition & 0 deletions lib/less/tree/ruleset.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ var Ruleset = function (selectors, rules, strictImports, visibilityInfo) {
this._lookups = {};
this.strictImports = strictImports;
this.copyVisibilityInfo(visibilityInfo);
this.allowRoot = true;
};
Ruleset.prototype = new Node();
Ruleset.prototype.type = "Ruleset";
Expand Down
11 changes: 2 additions & 9 deletions lib/less/visitors/to-css-visitor.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,15 +214,8 @@ ToCSSVisitor.prototype = {
throw { message: "Function '" + ruleNode.name + "' is undefined",
index: ruleNode.index, filename: ruleNode.currentFileInfo && ruleNode.currentFileInfo.filename};
}
if (!ruleNode.isRulesetLike &&
(ruleNode.type !== "Rule") &&
(ruleNode.type !== "Comment") &&
(ruleNode.type !== "Extend") &&
(ruleNode.type !== "Import")) {
// I'm not quite sure about message, but considering these values can be here only
// as result of a "root-funtion" call, something close to
// "A value returned by function is unexpected here" or so could be more friendly
throw { message: "Invalid '" + ruleNode.type + "' value in a ruleset or root node",
if (!ruleNode.isRulesetLike && !ruleNode.allowRoot) {
throw { message: ruleNode.type + " returned by a function is not valid here.",
index: ruleNode.index, filename: ruleNode.currentFileInfo && ruleNode.currentFileInfo.filename};
}
}
Expand Down

7 comments on commit b67403d

@matthew-dean
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seven-phases-max Does this look correct?

@seven-phases-max
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.
Except that (if I'm not mistaken) if we also change this isRulesetLike flag to allowRoot, we won't anymore need !ruleNode.isRulesetLike in this condition at all.
This flag for Anonymous ctor is set only by @import (inline) result.

@matthew-dean
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I'll take a look at that (or you, whoever's first, lol). And then, probably for tests, we'll need to test pass / fail for nodes created by functions.

@matthew-dean
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking at the code, and the isRulesetLike property doesn't make sense to me. In Anonymous and Directive nodes, this is a function; in other places it's a boolean. If we just test ruleNode.isRulesetLike, then those two nodes will return true no matter what, since the function exists.

What is this for, and what is it supposed to do?

@seven-phases-max
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then those two nodes will return true no matter what

Yes, and that's what I used it for there. Basically in my code that was a shortcut for != Directive || !=Anonymous || != Ruleset).
As for making sence, that's why I mentioned it's "broken" there. I suppose initially it was meant to mean exactly what allowRoot does, but later different people started to use it for different and even contradictory purposes (for example notice that in https://github.com/less/less.js/blob/master/lib/less/tree/directive.js#L39 the function returns nothing but just Directive.name !== "@charset" :)

@matthew-dean
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and that's what I used it for there.

Er..... okay. That's just hard to follow logically, because ruleNode.isRulesetLike will return true (or eval to true) even if ruleNode.isRulesetLike() returns false. I assume you mean what you are saying, but then why not be consistent with isRulesetLike and use it as a boolean across all nodes, and just set a default value?

Are we really sometimes setting that property to a function to return a value, sometimes as a value, and sometimes using it as a "truthy" evaluation? That just doesn't seem right. Do you know what I mean?

@seven-phases-max
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but then why not be consistent with isRulesetLike and use it as a boolean across all nodes, and just set a default value?

Just because this would mean you have to find all code where it's used and fix/refactor it to behave consistently :) It may be not so easy and it's just beyond the scope of the PR :) That's why above I suggested to get rid of it at all (just by changing it within Anonymous as it's the only place where we would care of it for the purposes of this branch).

Please sign in to comment.