Add `opts.preserveInjectedVariables`

[akdetrick]

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);
}

[MadLittleMods]

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

[akdetrick]

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: 1) you define global custom properties in a base stylesheet or inline style 2) you pass the same custom properties via JS to this postcss plugin 3) 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]

@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]

@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]

@akdetrick This comment is still outstanding, https://github.com/MadLittleMods/postcss-css-variables/pull/74#discussion_r197321742

[akdetrick]

@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]

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

[MadLittleMods]

@akdetrick Thanks for the contribution ❤️

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

[akdetrick]

🙌 thank you!