Merge pull request #96 from willofindie/fix/issue-95

phoenisx · web-flow · commit 52ea4c7d2ca1 · 2022-11-06T08:54:34.000+05:30
[Fix] Handle variable recursive evaluation
diff --git a/examples/multi-root/proj-1/index.css b/examples/multi-root/proj-1/index.css
@@ -7,6 +7,9 @@
   color: var(--color-1);
   background-color: var(--color-2);
 
+  /* LOL!! I didn't know adding a var() in the comment makes it visible for diagnostics */
+  /* FIXME(phoenisx) When var() function is used with default values, the regex doesn't */
+  /* work as expected */
   /* Non declared variable test */
   color: var(--space-2, 2rem);
 }
diff --git a/examples/multi-root/proj-3/index.css b/examples/multi-root/proj-3/index.css
@@ -2,6 +2,8 @@
 @import "./theme.dark.css";
 
 body {
+  /* FIXME(phoenisx) Not working. It was working before */
+  /* It started working after reload */
   color: var(--primary-text);
   background-color: var(--primary-bg);
   padding: var(--container-spacing);
diff --git a/examples/recursion/.vscode/settings.json b/examples/recursion/.vscode/settings.json
@@ -0,0 +1,3 @@
+{
+  "cssvar.files": ["testing-*.css"],
+}
diff --git a/examples/recursion/01-defined-early.css b/examples/recursion/01-defined-early.css
@@ -0,0 +1,12 @@
+:root {
+  --foo: #f00;
+  --bar: var(--foo);
+}
+
+.test {
+  background: var(--foo);
+}
+
+.test--two {
+  --foo: var(--bar);
+}
diff --git a/examples/recursion/02-var-fn-not-evaluated.css b/examples/recursion/02-var-fn-not-evaluated.css
@@ -0,0 +1,17 @@
+:root {
+  --foo: #f00;
+  --joo: 4px;
+  /* Following does not get normalised in post-processing, because of regex */
+  --too: 2px var(--joo);
+  --bar: var(--too);
+}
+
+.test {
+  /* --foo should point to #f00, but it isn't righ now */
+  /* Need to find a way to set recursive value to previously declared variable */
+  background: var(--foo);
+}
+
+.test--two {
+  --foo: var(--bar);
+}
diff --git a/examples/recursion/03-no-value-defined.css b/examples/recursion/03-no-value-defined.css
@@ -0,0 +1,12 @@
+/* Folllowing is valid css */
+:root {
+  --bar: var(--foo);
+}
+
+.test {
+  background: var(--foo);
+}
+
+.test--two {
+  --foo: var(--bar);
+}
diff --git a/examples/recursion/04-defined-value-after-use.css b/examples/recursion/04-defined-value-after-use.css
@@ -0,0 +1,16 @@
+/* Folllowing is valid css */
+:root {
+  --bar: var(--foo);
+}
+
+.test {
+  background: var(--foo);
+}
+
+.test--two {
+  --foo: var(--bar);
+}
+
+.test--two.define {
+  --foo: lightgreen;
+}
diff --git a/examples/recursion/testing-base.css b/examples/recursion/testing-base.css
@@ -0,0 +1,12 @@
+:root {
+  --fuzz: #fff;
+
+  --fuzz-large: var(--fuzz);
+  --boo: var(--not-defined);
+}
+
+@media (min-width: 768px) {
+  :root {
+    --boo: 48px;
+  }
+}
diff --git a/examples/recursion/testing-index.css b/examples/recursion/testing-index.css
@@ -0,0 +1,10 @@
+.card {
+  background: var(--fuzz);
+  padding: var(--not-defined);
+}
+
+/* Just updating variables instead of having to re-define each property */
+.card--large {
+  --fuzz: var(--fuzz-large);
+  --not-defined: var(--boo);
+}
diff --git a/examples/recursion/testing-solo.css b/examples/recursion/testing-solo.css
@@ -0,0 +1,16 @@
+/* Folllowing is valid css */
+:root {
+  --bar: var(--foo);
+}
+
+.test {
+  background: var(--foo);
+}
+
+.test--two {
+  --foo: var(--bar);
+}
+
+.test--two.define {
+  --foo: lightgreen;
+}
diff --git a/examples/tokencss-plugin/.vscode/settings.json b/examples/tokencss-plugin/.vscode/settings.json
@@ -1,5 +1,6 @@
 {
   "cssvar.postcssPlugins": [
+    // `cwd` option is required only if tokencss config is not declared directly under project root
     ["@tokencss/postcss", { "cwd": "examples/tokencss-plugin/config" }]
   ]
 }
diff --git a/src/test/main.test.ts b/src/test/main.test.ts
@@ -223,8 +223,3 @@ describe("Test Extension Main", () => {
     });
   });
 });
-
-// Need to work on this later.
-test.skip("setup() should map user config to in-mem Config object", () => {
-
-});
diff --git a/src/utils.ts b/src/utils.ts
@@ -14,6 +14,7 @@ import {
   SupportedLanguageIds,
   VAR_KEYWORD_REVERSE,
   CACHE,
+  PATTERN_ALL_VARIABLE_USAGES,
 } from "./constants";
 import { CSSVarDeclarations } from "./main";
 import { serializeColor } from "./color-parser";
@@ -100,17 +101,26 @@ export const populateValue = async (
   // Get Color for each, and modify the cssVar Record.
   const vars = getCSSDeclarationArray(cssVars);
 
+  // TODO(phoenisx) Remove this state.
   const cssVarsMapToSelf = vars.reduce((defs, cssVar) => {
     defs[cssVar.property] = cssVar;
     return defs;
   }, {} as Record<string, CSSVarDeclarations>);
 
-  // [TODO(shub)] Improve the following code, if possible
   // Mutating self inside the loop is not performant
+  /**
+   * [TODO(shub)] Improve the following code:
+   * - There can be duplicate variables; thus once value is found:
+   *    - We can either skip any subsequent loops for the same variable.
+   *    - Or make subsequent variable values scope aware. I mean decipher
+   *      nearset value set for that variable. This is too difficult, because it depends
+   *      on HTML layout and what parent a rule will reside under. (Skip)
+   * - `getValue` function should be memoized once the recursion starts.
+   */
   for await (const cssVar of vars) {
     const isVariable = getVariableType(cssVar.value);
     if (typeof isVariable === "string") {
-      const value = getValue(getVariableName(cssVar.value), cssVarsMapToSelf);
+      const value = getValue(getVariableName(cssVar.value), vars);
       cssVar.value = value || cssVar.value;
     }
 
@@ -124,16 +134,28 @@ export const populateValue = async (
 };
 
 function getValue(
-  name: string,
-  cssVarsMapToSelf: Record<string, CSSVarDeclarations | null>
+  prop: string, // Points to CSS variable name (a.k.a Propname in a CSS declarations)
+  cssVars: CSSVarDeclarations[]
 ): string | null {
-  const currentCssVar = cssVarsMapToSelf[name];
+  const currentCssVar = cssVars.reduce<CSSVarDeclarations | null>(
+    (currentCssVar, cssVar) => {
+      if (
+        !currentCssVar &&
+        cssVar.property === prop &&
+        !cssVar.value.match(PATTERN_ALL_VARIABLE_USAGES)
+      ) {
+        currentCssVar = cssVar;
+      }
+      return currentCssVar;
+    },
+    null
+  );
   if (currentCssVar) {
     const variableType = getVariableType(currentCssVar.value);
     if (variableType === null) {
       return currentCssVar.value;
     } else {
-      return getValue(getVariableName(currentCssVar.value), cssVarsMapToSelf);
+      return getValue(getVariableName(currentCssVar.value), cssVars);
     }
   }
 

Original file line number	Diff line number	Diff line change
`@@ -7,6 +7,9 @@`
`7`	`7`	`color: var(--color-1);`
`8`	`8`	`background-color: var(--color-2);`
`9`	`9`
	`10`	`+ /* LOL!! I didn't know adding a var() in the comment makes it visible for diagnostics */`
	`11`	`+ /* FIXME(phoenisx) When var() function is used with default values, the regex doesn't */`
	`12`	`+ /* work as expected */`
`10`	`13`	`/* Non declared variable test */`
`11`	`14`	`color: var(--space-2, 2rem);`
`12`	`15`	`}`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+{`
	`2`	`+ "cssvar.files": ["testing-*.css"],`
	`3`	`+}`