Add `opts.preserveInjectedVariables` #74

akdetrick · 2018-06-12T20:27:01Z

What is this new option?

This PR adds an option, preserveInjectedVariables, which when set to false, removes custom property
declarations added via opts.variables.

Why is this option useful?

The current behavior of opts.variables is to write a :root rule block containing custom property declarations passed from opts.variables when preserve is set to true.

When working with a single stylesheet, this behavior is exactly what you would want when using
the preserve option. Custom property declarations are added to output CSS so that preserved var() declarations can be resolved by a modern browser.

When working with CSS Modules, injected custom properties are repeated in every style module.

This cleanInjectedVariables option gives users control over where global custom property definitions live. Users can pass opts.variables so postcss-css-variables can resolve fallbacks for older browsers, and provide custom property declarations in a global stylesheet so that modern browsers can resolve var() at runtime without the need to repeat custom property declarations in every module/stylesheet transformed by this plugin.

Example

Usage in plugin options

require('postcss-css-variables')({
	preserve: true,
	preserveInjectedVariables: false,
	variables: {
		'--c-black': '#000',
		'--c-white': '#fff',
		'--font-size-big': '32px',
		...50 more variables...
	}
})

`someModule.module.css` transformed with `preserveInjectedVariables: true`

// injected declarations in every module.
:root {
	--c-black: #000;
	--c-white: #fff;
	--font-size-big: 32px;
	...50 more declarations...
}

.someElement {
	background: #000;
	background: var(--c-black);
	color: #fff;
	color: var(--c-white);
	font-size: 32px;
	font-size: var(--font-size-big);
}

`someModule.module.css` transformed with `cleanInjectedVariables: false`

// injected declarations from `opts.variables` are removed, but fallbacks
// can still be calculated, allowing the developer to include global custom
// property declarations outside of individual modules.

.someElement {
	background: #000;
	background: var(--c-black);
	color: #fff;
	color: var(--c-white);
	font-size: 32px;
	font-size: var(--font-size-big);
}

add option to clean from output variables injected via js add option to clean from output variables injected via js

MadLittleMods · 2018-06-12T21:30:15Z

Was this inspired by the conversation here https://gitter.im/postcss/postcss?at=5b1eae1582b1b355c94e9efa ? cc @killtheliterate

akdetrick · 2018-06-13T14:55:43Z

It was not inspired by that conversation, but probably covers the issue. If I understand the discussion, it looks like there's a use case for "global vars I want everywhere" without duplicating the rules or importing them.

This new option would provide a solution in this form:

you define global custom properties in a base stylesheet or inline style
you pass the same custom properties via JS to this postcss plugin
fallbacks can be generated for files run though the plugin, but custom property definitions are not added by the plugin (they're already available in the browser global scope)

akdetrick · 2018-06-18T22:13:24Z

@MadLittleMods Does this option make sense to merge into the mainline project, or should we plan on using a fork to meet our need of stripping js-injected -- custom property definitions from output?

As a maintainer, you might have a better idea how common this use case would be.

MadLittleMods

.

MadLittleMods · 2018-06-22T01:38:14Z

index.js

+	variables: {},
+	// Remove variables injected via JS with the `variables` option above
+	// before serializing to CSS
+	cleanInjectedVariables: false


I think we should call this preserveInjectedVariables: true

MadLittleMods · 2018-06-22T01:50:10Z

index.js

@@ -106,6 +113,12 @@ module.exports = postcss.plugin('postcss-css-variables', function(options) {
 				});
 				variableRootRule.append(varDecl);

+				// mark JS-injected variables for removal if `cleanInjectedVariables`


Just above lines 106-114, we insert a root note with the variables. We can probably just add an if-statement if(!opts.preserveInjectedVariables) instead of having to remove the decls we just added.

I'm waiting until the end to remove injected variable declarations so that they're available in the AST to calculate fallbacks for var() declarations as expected.

@akdetrick I think the map should be sufficient though? We don't look in the AST for variables after finding them all in the beginning

MadLittleMods · 2018-06-22T01:52:20Z

test/test.js

+		);
+		test(
+			'should preserve var() values and clean injected declarations with `options.variables` AND `options.preserve` AND `options.cleanInjectedVariables`',
+			'js-defined-preserve-cleanInjected',


js-defined-preserve-injected-false

MadLittleMods · 2018-06-22T01:56:29Z

@akdetrick Thanks for the contribution. I added some review comments on a alternative approach.

It seems like this use-case could be covered by css-nano discardDuplicates optimisation,

MadLittleMods · 2018-06-25T16:35:08Z

README.md

@@ -368,6 +368,12 @@ Can be a simple key-value pair or an object with a `value` property and an optio

 The object keys are automatically prefixed with `--` (according to CSS custom property syntax) if you do not provide it.

+### `cleanInjectedVariables` (default: `false`)


preserveInjectedVariables (default: true)

MadLittleMods · 2018-06-25T16:35:31Z

README.md

@@ -368,6 +368,12 @@ Can be a simple key-value pair or an object with a `value` property and an optio

 The object keys are automatically prefixed with `--` (according to CSS custom property syntax) if you do not provide it.

+### `cleanInjectedVariables` (default: `false`)
+
+Removes custom property declarations inserted via the `variables` option from final output.


Whether to preserve the ....

MadLittleMods · 2018-06-25T17:29:24Z

@akdetrick This comment is still outstanding, #74 (comment)

akdetrick · 2018-06-26T18:40:29Z

@MadLittleMods Thanks for your attention on this.

Regarding #74 - I agree it's preferable to avoid adding js-injected vars to the AST in the first place, but it may involve a non-trivial refactor of the tools in lib...

Consider the parent property in the expected map value for each js-injected variable:

	// Add the entry to the map
	prevVariableMap[variableName] = (prevVariableMap[variableName] || []).concat({
		decl: varDecl,
		prop: variableName,
		calculatedInPlaceValue: variableValue,
		isImportant: isImportant,
		variablesUsed: [],
		parent: variableRootRule, // Expects an AST node
		isUnderAtRule: false
	});

As the parent property is expecting an AST node, so a best case scenario would be to always write the parent node, but only conditionally append the declaration for each js-injected variable:

	// Add a root node to the AST
	var variableRootRule = postcss.rule({ selector: ':root' });
	css.root().prepend(variableRootRule);

	// Add the variable decl to the root node
	var varDecl = postcss.decl({
		prop: variableName,
		value: variableValue
	});

	// only append the declaration for the var if `preserveInjectedVariables` is `true`
	if (opts.preserveInjectedVariables) {
		variableRootRule.append(varDecl);
	}

This means that parent would be defined as an AST node, but the node will be empty. However, the plugin is expecting this to be a non-empty AST node. The map object doesn't exactly work separately from the AST; it holds references to AST nodes. Testing the code above will result in this error (likely one of many to fix):

     TypeError: Cannot read property 'parent' of undefined
      at findNodeAncestorWithSelector (lib/find-node-ancestor-with-selector.js:11:20)

tl;dr
The map object holds references to AST and relies on the js-injected variables to be present nodes in the AST. I'd vote for following the convention of nodesToRemoveAtEnd, which allows nodes to be written to the AST and cleans them up after transforms have completed.

MadLittleMods

Small comment touch-ups

MadLittleMods · 2018-06-26T19:12:37Z

index.js

@@ -82,6 +85,10 @@ module.exports = postcss.plugin('postcss-css-variables', function(options) {
 		// We use this because we don't want to modify the AST when we still need to reference these later on
 		var nodesToRemoveAtEnd = [];

+		// List of declarations injected from `opts.variables` to remove at the end
+		// if user passes `opts.preserveInjectedVariables`


// if user passes opts.preserveInjectedVariables = false

MadLittleMods · 2018-06-26T19:12:57Z

index.js

@@ -106,6 +113,12 @@ module.exports = postcss.plugin('postcss-css-variables', function(options) {
 				});
 				variableRootRule.append(varDecl);

+				// mark JS-injected variables for removal if `preserveInjectedVariables`
+				// option is passed.


// Colect JS-injected variables for removal if opts.preserveInjectedVariables = false

MadLittleMods · 2018-06-26T19:14:02Z

@akdetrick Thanks for trying the alternative. We can go with it for now 🙇

`opts.preserveInjectedVariables = false` means remove JS injected variables

MadLittleMods · 2018-06-26T19:28:05Z

@akdetrick Thanks for the contribution ❤️

Shipped in postcss-css-variables@0.9.0 and is available on npm 🚀

akdetrick · 2018-06-26T19:34:53Z

🙌 thank you!

This Pull Request updates dependency [postcss-css-variables](https://github.com/MadLittleMods/postcss-css-variables) from `v0.8.1` to `v0.9.0` <details> <summary>Release Notes</summary> ### [`v0.9.0`](https://github.com/MadLittleMods/postcss-css-variables/blob/master/CHANGELOG.md#v090---2018-6-26) [Compare Source](MadLittleMods/postcss-css-variables@v0.8.1...v0.9.0) ### v0.9.0 - 2018-6-26 - Adds `opts.preserveInjectedVariables`, which when set to `false`, removes the `:root { ... }` custom property declarations added via `opts.variables` - Thank you to [@akdetrick] for the [contribution](`https://github.com/MadLittleMods/postcss-css-variables/pull/74`) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com).

works

77c7974

add option to clean from output variables injected via js add option to clean from output variables injected via js

MadLittleMods requested changes Jun 22, 2018

View reviewed changes

MadLittleMods added the enhancement label Jun 22, 2018

rename new option to preserveInjectedVariables

92f986a

MadLittleMods reviewed Jun 25, 2018

View reviewed changes

update code comments and README

ee6f3d4

MadLittleMods changed the title ~~add cleanInjectedVariables option~~ Add opts.preserveInjectedVariables Jun 26, 2018

MadLittleMods requested changes Jun 26, 2018

View reviewed changes

Update comments to straightforward when they apply

0e06dde

`opts.preserveInjectedVariables = false` means remove JS injected variables

MadLittleMods approved these changes Jun 26, 2018

View reviewed changes

Correct "collect" typo

d969b4a

MadLittleMods merged commit 46a3b8e into MadLittleMods:master Jun 26, 2018

		@@ -368,6 +368,12 @@ Can be a simple key-value pair or an object with a `value` property and an optio

		The object keys are automatically prefixed with `--` (according to CSS custom property syntax) if you do not provide it.

		### `cleanInjectedVariables` (default: `false`)

Add opts.preserveInjectedVariables #74

Add opts.preserveInjectedVariables #74

Uh oh!

Conversation

akdetrick commented Jun 12, 2018 • edited by MadLittleMods Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this new option?

Why is this option useful?

Example

Usage in plugin options

someModule.module.css transformed with preserveInjectedVariables: true

someModule.module.css transformed with cleanInjectedVariables: false

Uh oh!

MadLittleMods commented Jun 12, 2018

Uh oh!

akdetrick commented Jun 13, 2018

Uh oh!

akdetrick commented Jun 18, 2018

Uh oh!

MadLittleMods left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods commented Jun 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods commented Jun 25, 2018

Uh oh!

akdetrick commented Jun 26, 2018

Uh oh!

MadLittleMods left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MadLittleMods commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akdetrick commented Jun 26, 2018

Uh oh!

Uh oh!

Add `opts.preserveInjectedVariables` #74

Add `opts.preserveInjectedVariables` #74

akdetrick commented Jun 12, 2018 •

edited by MadLittleMods

Loading

`someModule.module.css` transformed with `preserveInjectedVariables: true`

`someModule.module.css` transformed with `cleanInjectedVariables: false`

MadLittleMods Jun 26, 2018 •

edited

Loading

MadLittleMods commented Jun 26, 2018 •

edited

Loading

MadLittleMods commented Jun 26, 2018 •

edited

Loading