Fix regex in resolve-value.js to allow nested CSS functions #97

juliovedovatto · 2019-04-16T21:40:58Z

This fix will make work, when using nested CSS functions in the same declaration.

See #96

This fix will make work, when using nested CSS functions in the same declaration. See #96

package.json

lib/resolve-value.js

MadLittleMods

Thanks for taking a look at this!

Needs some new test cases and pass existing tests

juliovedovatto · 2019-04-24T04:12:30Z

First, thank you @MadLittleMods for the opportunity to let me help with this small fix 😄

My Scenario:

Previously, I just did a hotfix, without testing it properly, because deadline was short. Due IE11 browser, we had to convert every var() to its own original value. We used https://material.io grid layout and it contains A LOT of CSS variables.

My new merge proposal:

I decided to change my approach, and improved the RegExp sentence. I added an extra instruction to help match nested functions with var() declarations. With a fallback using the original regex block.

In addition, I had to change your approach on how replace results. I added a while block (not sure if is a good idea), to replace until no var() is found.

To finish, I just updated nested-inside-other-func test, with two situations: one with a nested function with a declared var() and another with a undefined var(). Using the orginal RegExp it fails, using my proposal, it passes 🎉

Please check if my logic makes sense to you and if you can imagine any extra scenario to test.

Cheers from Brazil 🇧🇷

MadLittleMods

A lot of change in the diff because of the new indentation. If there were any changes inside, please point them out with some comments

lib/resolve-value.js

test/fixtures/nested-inside-other-func.css

…iables with CSS functions) and added few more tests.

…, using match-recursive libr .editorconfig removed

juliovedovatto · 2019-04-26T01:02:43Z

@MadLittleMods I made more code changes, just to adapt your logic in some special scenarios.

I was facing issues with RegExp to match few nested inside css functions.

I spent a good time trying to create a good regex to help, but due the fact JS RegExp engine is limited, I had to change my approach.

The match-recursive npm lib was added, to help match several var() declaration in the same line. Additionally, I changed a few chunks of the code to fit my logic into your code. Also, I changed the original regex in RE_VAR_FUNC variable, just to help fill variablesUsedInValue array.

I added more related tests and worked ok.

Could you take a look and make some tests? I believe this is a good candidate to merge 🤓

Cheers!

package.json preinstall instruction added, to avoid npm not installing required package dependencies.

…more tests added for css nested functions.

juliovedovatto · 2019-04-29T22:50:34Z

@MadLittleMods I did code changes again, please review. I think we are in the right track :)

Now, it will use a while loop and balanced-match npm lib to match var() declarations and resolve them. This should cover nested CSS functions.

In addition, I added one more test for css nested functions. It should be all covered now.

Cheers!

MadLittleMods

Thanks for the changes and extra tests for our confidence. Some new review points to address

package.json

lib/resolve-value.js

test/fixtures/nested-inside-other-func.css

This reverts commit 9088a86.

… versions

juliovedovatto · 2019-05-14T01:49:12Z

@MadLittleMods Another code review round done. Please check.

juliovedovatto · 2019-05-27T19:41:37Z

@MadLittleMods any updates?

MadLittleMods

Sorry for the delay. Here is the latest

lib/resolve-value.js

colinhowells · 2019-06-01T07:03:21Z

Thanks everyone for your hard work on this – I've been watching this bit of the HiQ framework

--radio-border-width: var(--hiq-radio-border-width, 1px) solid var(--hiq-radio-border-color, transparent);
--radio-border-color: var(--hiq-radio-border-color, transparent);
border: var(--radio-border-width) solid var(--radio-border-color);

get compiled to:

border: 1px) solid var(--hiq-radio-border-color, transparent solid transparent;

and had no clue why. If it was defeated by regex, that'd make sense ...

…anced npm library

juliovedovatto · 2019-06-02T00:33:34Z

@MadLittleMods Done with the requested changes. Could you take a look? :)

@colinhowells You are right, it was defeated by regex. The intend of my proposal is to fix when some complex var() usage, specially when nested declarations appear.

lib/resolve-value.js

MadLittleMods · 2019-06-22T00:30:41Z

lib/resolve-value.js

@@ -27,26 +29,61 @@ function toString(value) {
 var resolveValue = function(decl, map, /*optional*/ignorePseudoScope, /*internal debugging*/_debugIsInternal) {
 	var debugIndent = _debugIsInternal ? '\t' : '';

+	var matchingVarDecl = undefined;
+	var RE_VAR_FUNC_G = new RegExp(RE_VAR_FUNC.source, 'g');


We can get rid of RE_VAR_FUNC_G and RE_VAR_FUNC now

RE_VAR_FUNC_G was removed.

But, It seems we can't remove RE_VAR_FUNC, since it is being called in index.js.

postcss-css-variables/index.js

Lines 216 to 227 in 5695b77

var doesRuleUseVariables = rule.nodes.some(function(node) {

if(node.type === 'decl') {

var decl = node;

// If it uses variables

// and is not a variable declarations that we may be preserving from earlier

if(resolveValue.RE_VAR_FUNC.test(decl.value) && !RE_VAR_PROP.test(decl.prop)) {

return true;

}

}

return false;

});

lib/resolve-value.js

MadLittleMods

Looking good, just a few remaining points 👍

juliovedovatto · 2019-06-22T14:37:06Z

@MadLittleMods I did some of your requests, could you review again?

Also, check my comment #97 (diff)

juliovedovatto · 2019-07-16T15:19:49Z

@MadLittleMods any news?

juliovedovatto · 2019-08-15T13:23:48Z

@MadLittleMods sorry to bump this PR, but could you take a look in the latest changes sent?

lib/resolve-value.js

MadLittleMods · 2019-11-25T01:19:25Z

I've left this for a while. Just feeling the effects of going back and fourth with things not actually changing or being quite finished.

I only spotted one thing and the tests are passing so we can merge after that 😥

juliovedovatto · 2019-11-25T02:45:50Z

No worries @MadLittleMods

I just did the request change. Feel free to to approve ;)

MadLittleMods · 2019-11-25T04:12:50Z

Thanks for the quick update @juliovedovatto ❤️

This is now part of postcss-css-variables@0.14.0 🎉, https://github.com/MadLittleMods/postcss-css-variables/blob/master/CHANGELOG.md#v0140---2019-11-24

juliovedovatto added 2 commits April 16, 2019 18:40

fixed regex in resolve-value.js

adcc69c

This fix will make work, when using nested CSS functions in the same declaration. See #96

package.json version bump

398a88e

MadLittleMods reviewed Apr 17, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

MadLittleMods reviewed Apr 17, 2019

View reviewed changes

lib/resolve-value.js Outdated Show resolved Hide resolved

MadLittleMods requested changes Apr 17, 2019

View reviewed changes

MadLittleMods changed the title ~~fixed regex in resolve-value.js~~ Fix regex in resolve-value.js to allow nested CSS functions Apr 17, 2019

MadLittleMods assigned juliovedovatto Apr 17, 2019

MadLittleMods added the enhancement label Apr 17, 2019

juliovedovatto added 2 commits April 24, 2019 00:55

undo version bump

6ce15de

Fixed a problem with var() declarations in nested functions

dff2e3a

MadLittleMods requested changes Apr 24, 2019

View reviewed changes

lib/resolve-value.js Outdated Show resolved Hide resolved

test/fixtures/nested-inside-other-func.css Show resolved Hide resolved

juliovedovatto added 3 commits April 24, 2019 02:36

small code improvement to map css variables correctly (for nested var…

bd81a6f

…iables with CSS functions) and added few more tests.

.editorconfig added, to fix indentation problems (tab vs spaces)

bf638e1

Code improviments to match var() declarations in nested CSS functions…

cd5c5dc

…, using match-recursive libr .editorconfig removed

juliovedovatto added 3 commits April 26, 2019 14:18

Update package.json

9088a86

package.json preinstall instruction added, to avoid npm not installing required package dependencies.

Code improvements: changed regex match library to balanced-match and …

f3b8ea5

…more tests added for css nested functions.

Merge branch 'master' of https://github.com/Konnng/postcss-css-variables

5f51a60

MadLittleMods requested changes May 9, 2019

View reviewed changes

juliovedovatto added 3 commits May 9, 2019 10:01

Revert "Update package.json"

ba6e178

This reverts commit 9088a86.

Code refactor, to make lib more robust and compatible with older node…

94eb0aa

… versions

more use-case tests added

45ffc6c

MadLittleMods requested changes May 31, 2019

View reviewed changes

lib/resolve-value.js Outdated Show resolved Hide resolved

lib/resolve-value.js Outdated Show resolved Hide resolved

lib/resolve-value.js Outdated Show resolved Hide resolved

lib/resolve-value.js Outdated Show resolved Hide resolved

juliovedovatto added 2 commits June 1, 2019 20:45

Comment capitalization, to match the rest of the comments

5a75deb

Changed match strategy for variables names, to make better use of bal…

b3b3fb0

…anced npm library

MadLittleMods reviewed Jun 22, 2019

View reviewed changes

MadLittleMods requested changes Jun 22, 2019

View reviewed changes

Small changes and code cleanup

ca24d60

juliovedovatto mentioned this pull request Jun 27, 2019

Parenthesis missing when using variable with CSS functions (nested) #96

Closed

MadLittleMods reviewed Nov 25, 2019

View reviewed changes

lib/resolve-value.js Outdated Show resolved Hide resolved

Unused method removed

eb8adbe

MadLittleMods merged commit 046839f into MadLittleMods:master Nov 25, 2019

MadLittleMods added a commit that referenced this pull request Nov 25, 2019

Prepare changelog with #97

afc31f0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix regex in resolve-value.js to allow nested CSS functions #97

Fix regex in resolve-value.js to allow nested CSS functions #97

juliovedovatto commented Apr 16, 2019

MadLittleMods left a comment

juliovedovatto commented Apr 24, 2019

MadLittleMods left a comment

juliovedovatto commented Apr 26, 2019 •

edited

juliovedovatto commented Apr 29, 2019

MadLittleMods left a comment

juliovedovatto commented May 14, 2019

juliovedovatto commented May 27, 2019

MadLittleMods left a comment

colinhowells commented Jun 1, 2019

juliovedovatto commented Jun 2, 2019 •

edited

MadLittleMods Jun 22, 2019

juliovedovatto Jun 22, 2019

MadLittleMods left a comment

juliovedovatto commented Jun 22, 2019

juliovedovatto commented Jul 16, 2019

juliovedovatto commented Aug 15, 2019

MadLittleMods commented Nov 25, 2019

juliovedovatto commented Nov 25, 2019

MadLittleMods commented Nov 25, 2019

	var doesRuleUseVariables = rule.nodes.some(function(node) {
	if(node.type === 'decl') {
	var decl = node;
	// If it uses variables
	// and is not a variable declarations that we may be preserving from earlier
	if(resolveValue.RE_VAR_FUNC.test(decl.value) && !RE_VAR_PROP.test(decl.prop)) {
	return true;
	}
	}

	return false;
	});

Fix regex in resolve-value.js to allow nested CSS functions #97

Fix regex in resolve-value.js to allow nested CSS functions #97

Conversation

juliovedovatto commented Apr 16, 2019

MadLittleMods left a comment

Choose a reason for hiding this comment

juliovedovatto commented Apr 24, 2019

MadLittleMods left a comment

Choose a reason for hiding this comment

juliovedovatto commented Apr 26, 2019 • edited

juliovedovatto commented Apr 29, 2019

MadLittleMods left a comment

Choose a reason for hiding this comment

juliovedovatto commented May 14, 2019

juliovedovatto commented May 27, 2019

MadLittleMods left a comment

Choose a reason for hiding this comment

colinhowells commented Jun 1, 2019

juliovedovatto commented Jun 2, 2019 • edited

MadLittleMods Jun 22, 2019

Choose a reason for hiding this comment

juliovedovatto Jun 22, 2019

Choose a reason for hiding this comment

MadLittleMods left a comment

Choose a reason for hiding this comment

juliovedovatto commented Jun 22, 2019

juliovedovatto commented Jul 16, 2019

juliovedovatto commented Aug 15, 2019

MadLittleMods commented Nov 25, 2019

juliovedovatto commented Nov 25, 2019

MadLittleMods commented Nov 25, 2019

juliovedovatto commented Apr 26, 2019 •

edited

juliovedovatto commented Jun 2, 2019 •

edited