Improve compatibility with compound selectors #126

siilike · 2020-05-23T20:51:03Z

It's not perfect, but should be a significant improvement.

Also added special syntax for cases where explicit relations are not defined:

:root {
	--color: #000000;
}

body.bar * {
	--color: #ffffff;
}

#foo {
	color: var(--color);
}

which would output:

#foo {
	color: #000000;
}

body.bar #foo {
	color: #ffffff;
}

MadLittleMods · 2021-04-07T17:00:34Z

Thanks @siilike! Sorry for putting this off for so long. It would be good to have some review comments to describe the decisions and improvements but I understand it's been so long since you first implemented this.

It's great to see all of the new tests and existing tests still passing! I'll need to give this a slightly better look before merge.

MadLittleMods · 2021-04-07T17:01:15Z

lib/is-under-scope.js

@@ -1,3 +1,5 @@
+var log = require('debug')('postcss-css-variables:is-under-scope');


👍 on debug

MadLittleMods · 2021-04-07T17:07:02Z

package.json

@@ -28,7 +29,8 @@
    "eslint-plugin-react": "^7.1.0",
    "mocha": "^5.2.0",
    "postcss-discard-comments": "^4.0.0",
-    "postcss-normalize-whitespace": "^4.0.0"
+    "postcss-normalize-whitespace": "^4.0.0",


It doesn't look like postcss-normalize-whitespace is used anywhere. Let's remove if not needed

I assume you tried this out for the test comparison?

siilike · 2021-04-07T17:18:07Z

I actually have to admit that I'm not using this plugin any more due to being able to drop IE11 support.

It was used in production and I cannot really remember any issues.

As it's been so long since I made the changes I know exactly as much as you on how and why they were made. : )

MadLittleMods · 2021-04-07T17:21:01Z

lib/resolve-decl.js

@@ -9,16 +12,91 @@ var shallowCloneNode = require('./shallow-clone-node');
 var findNodeAncestorWithSelector = require('./find-node-ancestor-with-selector');
 var cloneSpliceParentOntoNodeWhen = require('./clone-splice-parent-onto-node-when');

+var parser = require('postcss-selector-parser');


Fantastic! I started trying to use this in https://github.com/MadLittleMods/postcss-selector-scope-utility to replace all of the logic here but never got it over the line.

MadLittleMods · 2021-04-07T17:23:05Z

lib/resolve-decl.js

+	}
+
+	return false;
+}


Let's add a comment describing how compatible and compatible2 are different and when you should use either

MadLittleMods · 2021-04-07T17:27:35Z

lib/resolve-decl.js

+					if(b < idx) {
+						// no match: already compatible or wildcard
+						if(i === 0) {
+							state = 1;


Can these states be nice string label constants or symbols? Reasoning about these arbitrary state numbers seems mind bending 🤯

MadLittleMods · 2021-04-07T17:33:26Z

lib/is-under-scope.js

-	// Remove the pseudo selectors from the nodeScope so it can match a broader version
-	// ex. `.foo:hover` can resolve variables from `.foo`
-	nodeScopeList = stripPseudoSelectorsFromScopeList(nodeScopeList);
+var isUnderScope = function(nodeScopeList, scopeNodeScopeList, /*optional*/ignorePseudo, /*optional*/keepPseudo) {


Can we document this dichotomy between ignorePseudo and keepPseudo? I'm not quite wrapping my head around why we need both options and them being opposite default states. Seems like they should both be ignoreX or keepX and maybe need better descriptors.

For example, just from an initial look, seems like ignorePseudo can cover keepPseudo by being ignorePseudo=false. But I understand they are probably tackling different use cases. So the parameter name should probably be different.

MadLittleMods · 2021-04-07T17:34:35Z

lib/resolve-value.js

-			// Make sure the variable declaration came from the right spot
-			// And if the current matching variable is already important, a new one to replace it has to be important
-			var isRoot = varDeclMapItem.parent.type === 'root' || varDeclMapItem.parent.selectors[0] === ':root';
+		var process = (ignorePseudo, ignorePseudo2) =>


This looks related to my comments about ignorePseudo/keepPseudo above, https://github.com/MadLittleMods/postcss-css-variables/pull/126/files#r608857080

We should comment about why 2?

Below, I see we do some matrix processing around these 2 options, would be good to comment about why and the combinations.

MadLittleMods · 2021-04-07T17:37:28Z

test/fixtures/compound10.css

+
+#foo li {
+	--color: #000000;
+}


👍 On writing a test case that should NOT happen!

MadLittleMods · 2021-04-07T17:40:16Z

test/fixtures/compound11.expected.css

+#foo a li {
+	color: #ffffff;
+}
+


It seems like this rule is missing from the expected output:

#foo.bar a + li { color: #000000; }

In an ideal world, should this be in the output? (is my thinking around this selector correct?)

Is this one of the known limitations? That's fine, but let's add a comment about it

MadLittleMods · 2021-04-07T17:45:23Z

test/fixtures/compound12.css

+	color: var(--color2);
+}
+
+#bar.bar a c b c + d e {


I assume this isn't in the output because #bar.bar a c b c does not match #bar a b and the sibling is off as well.

Let's add a comment above this case about why it does not match and we should not see it in the output.

MadLittleMods · 2021-04-07T17:47:41Z

test/fixtures/compound14.css

+
+#bar li {
+	color: var(--color2);
+}


It looks like the #foo, #bar, and #baz cases can be split into separate tests without affecting what we're trying to test. And makes them easier to understand when they fail.

MadLittleMods · 2021-04-07T17:50:43Z

test/fixtures/compound16.css

+	--color: #aaaaaa;
+}
+
+/* no selector created */


Let's improve this comment.

/* no selector created because it's targeting `a` and the rule with the color declaration is targeting `li` */

MadLittleMods · 2021-04-07T17:54:39Z

test/fixtures/compound17.css

+
+#foo1.bar ul div > li {
+	--color: #777777;
+}


Let's add a comment about why this rule should NOT match

MadLittleMods · 2021-04-07T17:58:35Z

test/fixtures/compound17.css

+
+#foo1.bar ul > div li {
+	--color: #888888;
+}


I'm going back and forth on whether this should match 🤔

From: #foo1 ul > li Matches: #foo1.bar ul > li Should this match?: #foo1.bar ul > div li

In any case, a comment about why or known-limitation

MadLittleMods · 2021-04-07T18:00:04Z

test/fixtures/compound17.css

+
+#foo3 ul + li {
+	color: var(--color);
+}


I think all of the #foo1, #foo2, and #foo3 rules can be split into their own tests

MadLittleMods · 2021-04-07T18:02:55Z

test/fixtures/compound20.css

+
+body.bar *:hover {
+	--color: #111111;
+}


This rule looks like a known-limitation and should be in the output. Let's add a comment

MadLittleMods · 2021-04-07T18:05:17Z

test/fixtures/compound5.css

+
+#foo.bar li {
+	--color: #000000;
+}


This seems like a known-limitation that should be in the output. Let's add a comment.

(maybe)

Or why it shouldn't be in output

MadLittleMods · 2021-04-07T18:08:50Z

test/fixtures/pseudo-selector-declare-variable.expected.css

@@ -2,6 +2,10 @@
 	color: #ff0000;
 }

+.bar:hover + .foo {
+	color: #f0f000;
+}


Nice! Happy to see this fixed now

MadLittleMods · 2021-04-07T18:15:45Z

lib/is-node-under-scope.js

@@ -1,11 +1,11 @@
 var isUnderScope = require('./is-under-scope');


I actually have to admit that I'm not using this plugin any more due to being able to drop IE11 support.

It was used in production and I cannot really remember any issues.

As it's been so long since I made the changes I know exactly as much as you on how and why they were made. : )

#126 (comment)

@siilike No worries, I understand. I might just add the comments to the tests where I pointed it out and merge this.

Seems like a decent improvement regardless of the code and us trying to understand it better.

tomsommer · 2023-10-12T12:49:25Z

Can this be merged?

siilike added 4 commits May 23, 2020 23:44

various improvements

7827c7c

add partial star selector support

55691c9

more fixes

976e605

fix cloning nodes with undefined values

d0cd034

MadLittleMods added the enhancement label Apr 7, 2021

MadLittleMods reviewed Apr 7, 2021

View reviewed changes

lib/is-under-scope.js

@@ -1,3 +1,5 @@

var log = require('debug')('postcss-css-variables:is-under-scope');

Copy link

Owner

MadLittleMods Apr 7, 2021

Choose a reason for hiding this comment

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

👍 on debug

MadLittleMods reviewed Apr 7, 2021

View reviewed changes

MadLittleMods changed the title ~~Various improvements~~ Improve compatibility with compound selectors Apr 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve compatibility with compound selectors #126

Improve compatibility with compound selectors #126

siilike commented May 23, 2020 •

edited by MadLittleMods

Loading

MadLittleMods commented Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021

MadLittleMods Apr 7, 2021 •

edited

Loading

siilike commented Apr 7, 2021

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021

MadLittleMods Apr 7, 2021

MadLittleMods Apr 7, 2021

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021

MadLittleMods Apr 7, 2021

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021

MadLittleMods Apr 7, 2021

tomsommer commented Oct 12, 2023

		@@ -1,3 +1,5 @@
		var log = require('debug')('postcss-css-variables:is-under-scope');

		@@ -1,11 +1,11 @@
		var isUnderScope = require('./is-under-scope');

Improve compatibility with compound selectors #126

Are you sure you want to change the base?

Improve compatibility with compound selectors #126

Conversation

siilike commented May 23, 2020 • edited by MadLittleMods Loading

MadLittleMods commented Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

MadLittleMods Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

siilike commented Apr 7, 2021

MadLittleMods Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadLittleMods Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

MadLittleMods Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

MadLittleMods Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadLittleMods Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadLittleMods Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadLittleMods Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

MadLittleMods Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomsommer commented Oct 12, 2023

siilike commented May 23, 2020 •

edited by MadLittleMods

Loading

MadLittleMods commented Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021 •

edited

Loading

MadLittleMods Apr 7, 2021 •

edited

Loading