From f0dc658933cc6c6faf565226fc9a6b9ae1977400 Mon Sep 17 00:00:00 2001 From: Abhishek Aroskar Date: Tue, 21 Oct 2025 12:17:23 -0700 Subject: [PATCH] Fix: Prevent duplicate media query blocks when resolving multiple variables When multiple CSS variables are used in a single property within a media query, the plugin was generating duplicate query blocks, causing significant CSS bloat. Root Cause: 1. eachMapItemDependencyOfDecl was passing entire variable list to gatherVariableDependencies on every iteration instead of single variable, causing duplication 2. Missing check to skip duplicate at-rules when variable already in scope Example (from issue #67): Input: ```css :root { --space-small: 10px; --space-large: 20px; } h1 { padding: var(--space-small) var(--space-large); } @media (min-width: 768px) { :root { --space-small: 20px; --space-large: 30px; } } ``` Before Fix (Buggy Output): ```css h1 { padding: 10px 20px; } @media (min-width: 768px) { h1 { padding: 20px 30px; } } @media (min-width: 768px) { h1 { padding: 20px 30px; } } @media (min-width: 768px) { h1 { padding: 20px 30px; } } @media (min-width: 768px) { h1 { padding: 20px 30px; } } ``` After Fix (Correct Output): ```css h1 { padding: 10px 20px; } @media (min-width: 768px) { h1 { padding: 20px 30px; } } ``` Solution: 1. Pass only current variable to gatherVariableDependencies 2. Skip creating duplicate at-rules when declaration and variable are in same at-rule context Impact: - Fixes issue #67 - Eliminates unecessary/duplicate @media blocks Testing: - Added 2 new test cases covering multiple variables in single property - All exiting tests are passing Closes #67 --- lib/resolve-decl.js | 14 ++++++++++- .../fixtures/multiple-vars-in-media-query.css | 24 +++++++++++++++++++ .../multiple-vars-in-media-query.expected.css | 12 ++++++++++ .../simple-multi-var-media-duplication.css | 13 ++++++++++ ...e-multi-var-media-duplication.expected.css | 6 +++++ test/test.js | 11 +++++++++ 6 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 test/fixtures/multiple-vars-in-media-query.css create mode 100644 test/fixtures/multiple-vars-in-media-query.expected.css create mode 100644 test/fixtures/simple-multi-var-media-duplication.css create mode 100644 test/fixtures/simple-multi-var-media-duplication.expected.css diff --git a/lib/resolve-decl.js b/lib/resolve-decl.js index e469849..07e4c4b 100644 --- a/lib/resolve-decl.js +++ b/lib/resolve-decl.js @@ -17,7 +17,7 @@ function eachMapItemDependencyOfDecl(variablesUsedList, map, decl, cb) { variablesUsedList.forEach(function(variableUsedName) { // Find anything in the map that corresponds to that variable - gatherVariableDependencies(variablesUsedList, map).deps.forEach(function(mapItem) { + gatherVariableDependencies([variableUsedName], map).deps.forEach(function(mapItem) { var mimicDecl; if(mapItem.isUnderAtRule) { @@ -96,6 +96,18 @@ function resolveDecl(decl, map, /*optional*/shouldPreserve, /*optional*/preserve //console.log('resolveDecl 2'); var previousAtRuleNode; eachMapItemDependencyOfDecl(valueResults.variablesUsed, map, decl, function(mimicDecl, mapItem) { + // Check if the declaration is already inside the same at-rule as the variable + var declAtRule = decl.parent.parent.type === 'atrule' ? decl.parent.parent : null; + var varAtRule = mapItem.isUnderAtRule ? mapItem.parent.parent : null; + var isSameAtRule = declAtRule && varAtRule && + declAtRule.name === varAtRule.name && + declAtRule.params === varAtRule.params; + + // Skip if the variable is in the same at-rule (already accessible) + if(isSameAtRule) { + return; + } + var ruleClone = shallowCloneNode(decl.parent); var declClone = decl.clone(); // Add the declaration to our new rule diff --git a/test/fixtures/multiple-vars-in-media-query.css b/test/fixtures/multiple-vars-in-media-query.css new file mode 100644 index 0000000..242b4a8 --- /dev/null +++ b/test/fixtures/multiple-vars-in-media-query.css @@ -0,0 +1,24 @@ +:root { + --translate-x: 0; + --translate-y: 0; + --rotate: 0deg; + --skew-x: 0deg; + --skew-y: 0deg; + --scale-x: 1; + --scale-y: 1; +} + +@media (max-width: 500px) { + .modal { + --translate-y: 100%; + transform: translate(var(--translate-x), var(--translate-y)) rotate(var(--rotate)) skewX(var(--skew-x)) skewY(var(--skew-y)) scaleX(var(--scale-x)) scaleY(var(--scale-y)); + } + + .drawer { + position: fixed; + --translate-y: -100%; + transform: translate(var(--translate-x), var(--translate-y)) rotate(var(--rotate)) skewX(var(--skew-x)) skewY(var(--skew-y)) scaleX(var(--scale-x)) scaleY(var(--scale-y)); + transition: transform 0.3s ease; + } +} + diff --git a/test/fixtures/multiple-vars-in-media-query.expected.css b/test/fixtures/multiple-vars-in-media-query.expected.css new file mode 100644 index 0000000..d6b178f --- /dev/null +++ b/test/fixtures/multiple-vars-in-media-query.expected.css @@ -0,0 +1,12 @@ +@media (max-width: 500px) { + .modal { + transform: translate(0, 100%) rotate(0deg) skewX(0deg) skewY(0deg) scaleX(1) scaleY(1); + } + + .drawer { + position: fixed; + transform: translate(0, -100%) rotate(0deg) skewX(0deg) skewY(0deg) scaleX(1) scaleY(1); + transition: transform 0.3s ease; + } +} + diff --git a/test/fixtures/simple-multi-var-media-duplication.css b/test/fixtures/simple-multi-var-media-duplication.css new file mode 100644 index 0000000..e0454b9 --- /dev/null +++ b/test/fixtures/simple-multi-var-media-duplication.css @@ -0,0 +1,13 @@ +:root { + --shadow-x: 0px; + --shadow-y: 2px; + --shadow-blur: 4px; +} + +@media (max-width: 500px) { + .card { + --shadow-y: 8px; + box-shadow: var(--shadow-x) var(--shadow-y) var(--shadow-blur) rgba(0, 0, 0, 0.1); + } +} + diff --git a/test/fixtures/simple-multi-var-media-duplication.expected.css b/test/fixtures/simple-multi-var-media-duplication.expected.css new file mode 100644 index 0000000..fd8c211 --- /dev/null +++ b/test/fixtures/simple-multi-var-media-duplication.expected.css @@ -0,0 +1,6 @@ +@media (max-width: 500px) { + .card { + box-shadow: 0px 8px 4px rgba(0, 0, 0, 0.1); + } +} + diff --git a/test/test.js b/test/test.js index e8f0af8..6ac701b 100644 --- a/test/test.js +++ b/test/test.js @@ -383,4 +383,15 @@ describe("postcss-css-variables", function() { "remove-nested-empty-rules-after-variable-collection" ); }); + + describe("bug fixes", function() { + test( + "should not duplicate media query blocks when resolving multiple variables (simple case)", + "simple-multi-var-media-duplication" + ); + test( + "should not duplicate media query blocks when resolving multiple variables in same property", + "multiple-vars-in-media-query" + ); + }); });