[FIX] Variables: Include variables defined in sub-directories (#160)

Fixes: #159

lib/plugin/variable-collector.js

@@ -1,6 +1,11 @@
 "use strict";
-const path = require("path");
+const posixPath = require("path").posix;
 const less = require("../thirdparty/less");
+const backslashRegExp = /\\/g;
+
+function toPosix(p) {
+	return p.replace(backslashRegExp, "/");
+}
 
 const VariableCollector = module.exports = function(env) {
 	/* eslint-disable new-cap */
@@ -43,15 +48,52 @@ VariableCollector.prototype = {
 	getVariables: function(aImports) {
 		const mVariables = {};
 
+		const aPosixImports = aImports.map(toPosix);
+
+		const includeCache = {};
+
+		function shouldIncludeVariable({rootFilename, filename}) {
+			// Simple case where variable is defined in root file
+			if (rootFilename === filename) {
+				return true;
+			}
+
+			const cacheKey = rootFilename + "|" + filename;
+			if (includeCache[cacheKey] !== undefined) {
+				return includeCache[cacheKey];
+			}
+
+			const dirname = posixPath.dirname(filename);
+			const baseFileName = posixPath.basename(rootFilename); // usually library.source.less
+
+			function check(currentDirname) {
+				const libraryBaseFile = posixPath.join(currentDirname, baseFileName);
+				if (libraryBaseFile === rootFilename || aPosixImports.includes(libraryBaseFile)) {
+					return true;
+				} else {
+					// Recursively check parent directories whether they contain an imported "base file"
+					// This is only relevant when a theme contains sub-directories
+					const parentDirname = posixPath.dirname(currentDirname);
+					if (parentDirname === currentDirname) {
+						return false; // We are at the root
+					} else {
+						return check(parentDirname);
+					}
+				}
+			}
+
+			return includeCache[cacheKey] = check(dirname);
+		}
+
 		for (const name in this.mVariables) {
 			if (Object.prototype.hasOwnProperty.call(this.mVariables, name)) {
 				const oVar = this.mVariables[name];
-				const dirname = path.posix.dirname(oVar.filename);
-				const baseFileName = path.posix.basename(oVar.rootFilename); // usually library.source.less
-				const libraryBaseFile = path.posix.normalize(path.posix.join(dirname, baseFileName));
+				// Ensure posix paths
+				const rootFilename = toPosix(oVar.rootFilename);
+				const filename = toPosix(oVar.filename);
 
 				// Only add variable if the corresponding library "base file" has been imported
-				if (aImports.indexOf(libraryBaseFile) > -1 || libraryBaseFile === oVar.rootFilename) {
+				if (shouldIncludeVariable({rootFilename, filename})) {
 					mVariables[name] = oVar.value;
 				}
 			}

test/expected/libraries/lib3/my/other/ui/lib/themes/bar/library-RTL.css

@@ -10,4 +10,4 @@
 }
 
 /* Inline theming parameters */
-#sap-ui-theme-my\.other\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23ffffff%22%2C%22_my_other_ui_lib_MyControl_color2%22%3A%22%23008000%22%7D%2C%22scopes%22%3A%7B%22barContrast%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23000000%22%7D%7D%7D')}
+#sap-ui-theme-my\.other\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23ffffff%22%2C%22_my_other_ui_lib_MyOtherControl_color1%22%3A%22%23ffffff%22%2C%22_my_other_ui_lib_MyControl_color2%22%3A%22%23008000%22%7D%2C%22scopes%22%3A%7B%22barContrast%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23000000%22%2C%22_my_other_ui_lib_MyOtherControl_color1%22%3A%22%23000000%22%7D%7D%7D')}

test/expected/libraries/lib3/my/other/ui/lib/themes/bar/library.css

@@ -10,4 +10,4 @@
 }
 
 /* Inline theming parameters */
-#sap-ui-theme-my\.other\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23ffffff%22%2C%22_my_other_ui_lib_MyControl_color2%22%3A%22%23008000%22%7D%2C%22scopes%22%3A%7B%22barContrast%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23000000%22%7D%7D%7D')}
+#sap-ui-theme-my\.other\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23ffffff%22%2C%22_my_other_ui_lib_MyOtherControl_color1%22%3A%22%23ffffff%22%2C%22_my_other_ui_lib_MyControl_color2%22%3A%22%23008000%22%7D%2C%22scopes%22%3A%7B%22barContrast%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23000000%22%2C%22_my_other_ui_lib_MyOtherControl_color1%22%3A%22%23000000%22%7D%7D%7D')}

test/expected/libraries/lib3/my/other/ui/lib/themes/base/library-RTL.css

@@ -6,4 +6,4 @@
 }
 
 /* Inline theming parameters */
-#sap-ui-theme-my\.other\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23fefefe%22%7D')}
+#sap-ui-theme-my\.other\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23fefefe%22%2C%22_my_other_ui_lib_MyOtherControl_color1%22%3A%22%23fefefe%22%7D')}

test/expected/libraries/lib3/my/other/ui/lib/themes/base/library.css

@@ -6,4 +6,4 @@
 }
 
 /* Inline theming parameters */
-#sap-ui-theme-my\.other\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23fefefe%22%7D')}
+#sap-ui-theme-my\.other\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23fefefe%22%2C%22_my_other_ui_lib_MyOtherControl_color1%22%3A%22%23fefefe%22%7D')}

test/expected/libraries/lib3/my/other/ui/lib/themes/foo/library-RTL.css

@@ -10,4 +10,4 @@
 }
 
 /* Inline theming parameters */
-#sap-ui-theme-my\.other\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23ffffff%22%2C%22_my_other_ui_lib_MyControl_color2%22%3A%22%23008000%22%7D%2C%22scopes%22%3A%7B%22fooContrast%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23000000%22%7D%7D%7D')}
+#sap-ui-theme-my\.other\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23ffffff%22%2C%22_my_other_ui_lib_MyOtherControl_color1%22%3A%22%23ffffff%22%2C%22_my_other_ui_lib_MyControl_color2%22%3A%22%23008000%22%7D%2C%22scopes%22%3A%7B%22fooContrast%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23000000%22%2C%22_my_other_ui_lib_MyOtherControl_color1%22%3A%22%23000000%22%7D%7D%7D')}

test/expected/libraries/lib3/my/other/ui/lib/themes/foo/library.css

@@ -10,4 +10,4 @@
 }
 
 /* Inline theming parameters */
-#sap-ui-theme-my\.other\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23ffffff%22%2C%22_my_other_ui_lib_MyControl_color2%22%3A%22%23008000%22%7D%2C%22scopes%22%3A%7B%22fooContrast%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23000000%22%7D%7D%7D')}
+#sap-ui-theme-my\.other\.ui\.lib{background-image:url('data:text/plain;utf-8,%7B%22default%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23ffffff%22%2C%22_my_other_ui_lib_MyOtherControl_color1%22%3A%22%23ffffff%22%2C%22_my_other_ui_lib_MyControl_color2%22%3A%22%23008000%22%7D%2C%22scopes%22%3A%7B%22fooContrast%22%3A%7B%22_my_other_ui_lib_MyControl_color1%22%3A%22%23000000%22%2C%22_my_other_ui_lib_MyOtherControl_color1%22%3A%22%23000000%22%7D%7D%7D')}

test/fixtures/libraries/lib3/my/other/ui/lib/themes/base/library.source.less

@@ -1,2 +1,3 @@
 @import "../../../../../../my/ui/lib/themes/base/global.less";
 @import "MyControl.less";
+@import "sub-directory/MyOtherControl.less";

test/fixtures/libraries/lib3/my/other/ui/lib/themes/base/sub-directory/MyOtherControl.less

@@ -0,0 +1 @@
+@_my_other_ui_lib_MyOtherControl_color1: @color1;

test/test-variable-collector.js

@@ -0,0 +1,115 @@
+/* eslint-env mocha */
+"use strict";
+
+const assert = require("assert");
+const VariableCollector = require("../lib/plugin/variable-collector");
+
+describe("VariableCollector", function() {
+	it("should return relevant variables when calling 'getVariables'", function() {
+		const env = {};
+		const variableCollector = new VariableCollector(env);
+
+		variableCollector.mVariables = {
+			"color1": {
+				"value": "#ffffff",
+				"filename": "my/ui/lib/themes/foo/global.less",
+				"rootFilename": "my/other/ui/lib/themes/foo/library.source.less"
+			},
+			"_my_other_ui_lib_MyControl_color1": {
+				"value": "#ffffff",
+				"filename": "my/other/ui/lib/themes/foo/MyControl.less",
+				"rootFilename": "my/other/ui/lib/themes/foo/library.source.less"
+			},
+			"_my_other_ui_lib_MyOtherControl_color1": {
+				"value": "#ffffff",
+				"filename": "my/other/ui/lib/themes/base/sub-directory/MyOtherControl.less",
+				"rootFilename": "my/other/ui/lib/themes/foo/library.source.less"
+			},
+			"_my_other_ui_lib_MyControl_color2": {
+				"value": "#008000",
+				"filename": "my/other/ui/lib/themes/foo/MyControl.less",
+				"rootFilename": "my/other/ui/lib/themes/foo/library.source.less"
+			}
+		};
+
+		const aImports = [
+			"my/ui/lib/themes/foo/global.less",
+			"my/other/ui/lib/themes/foo/MyControl.less",
+			"my/other/ui/lib/themes/base/library.source.less",
+			"my/ui/lib/themes/base/global.less",
+			"my/other/ui/lib/themes/base/MyControl.less",
+			"my/other/ui/lib/themes/base/sub-directory/MyOtherControl.less"
+		];
+
+		const variables = variableCollector.getVariables(aImports);
+
+		assert.deepEqual(variables, {
+			"_my_other_ui_lib_MyControl_color1": "#ffffff",
+			"_my_other_ui_lib_MyOtherControl_color1": "#ffffff",
+			"_my_other_ui_lib_MyControl_color2": "#008000"
+		}, "relevant variables should be returned");
+	});
+	it("should return relevant variables when calling 'getVariables' (win32 paths)", function() {
+		const env = {};
+		const variableCollector = new VariableCollector(env);
+
+		variableCollector.mVariables = {
+			"color1": {
+				"value": "#ffffff",
+				"filename": "my\\ui\\lib\\themes\\foo\\global.less",
+				"rootFilename": "my\\other\\ui\\lib\\themes\\foo\\library.source.less"
+			},
+			"_my_other_ui_lib_MyControl_color1": {
+				"value": "#ffffff",
+				"filename": "my\\other\\ui\\lib\\themes\\foo\\MyControl.less",
+				"rootFilename": "my\\other\\ui\\lib\\themes\\foo\\library.source.less"
+			},
+			"_my_other_ui_lib_MyOtherControl_color1": {
+				"value": "#ffffff",
+				"filename": "my\\other\\ui\\lib\\themes\\base\\sub-directory\\MyOtherControl.less",
+				"rootFilename": "my\\other\\ui\\lib\\themes\\foo\\library.source.less"
+			},
+			"_my_other_ui_lib_MyControl_color2": {
+				"value": "#008000",
+				"filename": "my\\other\\ui\\lib\\themes\\foo\\MyControl.less",
+				"rootFilename": "my\\other\\ui\\lib\\themes\\foo\\library.source.less"
+			}
+		};
+
+		const aImports = [
+			"my\\ui\\lib\\themes\\foo\\global.less",
+			"my\\other\\ui\\lib\\themes\\foo\\MyControl.less",
+			"my\\other\\ui\\lib\\themes\\base\\library.source.less",
+			"my\\ui\\lib\\themes\\base\\global.less",
+			"my\\other\\ui\\lib\\themes\\base\\MyControl.less",
+			"my\\other\\ui\\lib\\themes\\base\\sub-directory\\MyOtherControl.less"
+		];
+
+		const variables = variableCollector.getVariables(aImports);
+
+		assert.deepEqual(variables, {
+			"_my_other_ui_lib_MyControl_color1": "#ffffff",
+			"_my_other_ui_lib_MyOtherControl_color1": "#ffffff",
+			"_my_other_ui_lib_MyControl_color2": "#008000"
+		}, "relevant variables should be returned");
+	});
+	it("should return variables when calling 'getVariables' (filename = rootFilename, no imports)", function() {
+		const env = {};
+		const variableCollector = new VariableCollector(env);
+
+		variableCollector.mVariables = {
+			"color1": {
+				"value": "#ffffff",
+				"filename": "something",
+				"rootFilename": "something"
+			}
+		};
+		const aImports = [];
+
+		const variables = variableCollector.getVariables(aImports);
+
+		assert.deepEqual(variables, {
+			"color1": "#ffffff"
+		}, "relevant variables should be returned");
+	});
+});

test/test.js

@@ -241,15 +241,20 @@ describe("libraries (my/other/ui/lib)", function() {
 		}).then(function(result) {
 			assert.equal(result.css, readFile("test/expected/libraries/lib3/my/other/ui/lib/themes/base/library.css"), "css should be correctly generated.");
 			assert.equal(result.cssRtl, readFile("test/expected/libraries/lib3/my/other/ui/lib/themes/base/library-RTL.css"), "rtl css should be correctly generated.");
-			assert.deepEqual(result.variables, {"_my_other_ui_lib_MyControl_color1": "#fefefe"}, "variables should be correctly collected.");
+			assert.deepEqual(result.variables, {
+				"_my_other_ui_lib_MyControl_color1": "#fefefe",
+				"_my_other_ui_lib_MyOtherControl_color1": "#fefefe"
+			}, "variables should be correctly collected.");
 			assert.deepEqual(result.allVariables, {
 				"_my_other_ui_lib_MyControl_color1": "#fefefe",
+				"_my_other_ui_lib_MyOtherControl_color1": "#fefefe",
 				"color1": "#fefefe"
 			}, "allVariables should be correctly collected.");
 			assert.deepEqual(result.imports, [
 				path.join("test", "fixtures", "libraries", "lib3", "my", "other", "ui", "lib", "themes", "base", "library.source.less"),
 				path.join("test", "fixtures", "libraries", "lib1", "my", "ui", "lib", "themes", "base", "global.less"),
-				path.join("test", "fixtures", "libraries", "lib3", "my", "other", "ui", "lib", "themes", "base", "MyControl.less")
+				path.join("test", "fixtures", "libraries", "lib3", "my", "other", "ui", "lib", "themes", "base", "MyControl.less"),
+				path.join("test", "fixtures", "libraries", "lib3", "my", "other", "ui", "lib", "themes", "base", "sub-directory", "MyOtherControl.less")
 			], "import list should be correct.");
 		});
 	});
@@ -269,23 +274,27 @@ describe("libraries (my/other/ui/lib)", function() {
 			const oVariablesExpected = {
 				"default": {
 					"_my_other_ui_lib_MyControl_color1": "#ffffff",
-					"_my_other_ui_lib_MyControl_color2": "#008000"
+					"_my_other_ui_lib_MyControl_color2": "#008000",
+					"_my_other_ui_lib_MyOtherControl_color1": "#ffffff"
 				},
 				"scopes": {
 					"fooContrast": {
-						"_my_other_ui_lib_MyControl_color1": "#000000"
+						"_my_other_ui_lib_MyControl_color1": "#000000",
+						"_my_other_ui_lib_MyOtherControl_color1": "#000000"
 					}
 				}
 			};
 			const oAllVariablesExpected = {
 				"default": {
 					"_my_other_ui_lib_MyControl_color1": "#ffffff",
 					"_my_other_ui_lib_MyControl_color2": "#008000",
+					"_my_other_ui_lib_MyOtherControl_color1": "#ffffff",
 					"color1": "#ffffff"
 				},
 				"scopes": {
 					"fooContrast": {
 						"_my_other_ui_lib_MyControl_color1": "#000000",
+						"_my_other_ui_lib_MyOtherControl_color1": "#000000",
 						"color1": "#000000"
 					}
 				}
@@ -300,6 +309,7 @@ describe("libraries (my/other/ui/lib)", function() {
 				path.join("test", "fixtures", "libraries", "lib3", "my", "other", "ui", "lib", "themes", "base", "library.source.less"),
 				path.join("test", "fixtures", "libraries", "lib1", "my", "ui", "lib", "themes", "base", "global.less"),
 				path.join("test", "fixtures", "libraries", "lib3", "my", "other", "ui", "lib", "themes", "base", "MyControl.less"),
+				path.join("test", "fixtures", "libraries", "lib3", "my", "other", "ui", "lib", "themes", "base", "sub-directory", "MyOtherControl.less"),
 				path.join("test", "fixtures", "libraries", "lib1", "my", "ui", "lib", "themes", "foo", "global.less"),
 				path.join("test", "fixtures", "libraries", "lib3", "my", "other", "ui", "lib", "themes", "foo", "MyControl.less"),
 				path.join("test", "fixtures", "libraries", "lib3", "my", "other", "ui", "lib", "themes", "bar", "library.source.less"),
@@ -324,23 +334,27 @@ describe("libraries (my/other/ui/lib)", function() {
 			const oVariablesExpected = {
 				"default": {
 					"_my_other_ui_lib_MyControl_color1": "#ffffff",
-					"_my_other_ui_lib_MyControl_color2": "#008000"
+					"_my_other_ui_lib_MyControl_color2": "#008000",
+					"_my_other_ui_lib_MyOtherControl_color1": "#ffffff"
 				},
 				"scopes": {
 					"barContrast": {
-						"_my_other_ui_lib_MyControl_color1": "#000000"
+						"_my_other_ui_lib_MyControl_color1": "#000000",
+						"_my_other_ui_lib_MyOtherControl_color1": "#000000"
 					}
 				}
 			};
 			const oAllVariablesExpected = {
 				"default": {
 					"_my_other_ui_lib_MyControl_color1": "#ffffff",
 					"_my_other_ui_lib_MyControl_color2": "#008000",
+					"_my_other_ui_lib_MyOtherControl_color1": "#ffffff",
 					"color1": "#ffffff"
 				},
 				"scopes": {
 					"barContrast": {
 						"_my_other_ui_lib_MyControl_color1": "#000000",
+						"_my_other_ui_lib_MyOtherControl_color1": "#000000",
 						"color1": "#000000"
 					}
 				}
@@ -356,6 +370,7 @@ describe("libraries (my/other/ui/lib)", function() {
 				path.join("test", "fixtures", "libraries", "lib3", "my", "other", "ui", "lib", "themes", "base", "library.source.less"),
 				path.join("test", "fixtures", "libraries", "lib1", "my", "ui", "lib", "themes", "base", "global.less"),
 				path.join("test", "fixtures", "libraries", "lib3", "my", "other", "ui", "lib", "themes", "base", "MyControl.less"),
+				path.join("test", "fixtures", "libraries", "lib3", "my", "other", "ui", "lib", "themes", "base", "sub-directory", "MyOtherControl.less"),
 				path.join("test", "fixtures", "libraries", "lib1", "my", "ui", "lib", "themes", "foo", "global.less"),
 				path.join("test", "fixtures", "libraries", "lib3", "my", "other", "ui", "lib", "themes", "foo", "MyControl.less"),
 				path.join("test", "fixtures", "libraries", "lib2", "my", "ui", "lib", "themes", "bar", "global.less"),