Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Providing undefined as a string #44

Merged

Conversation

vincentorback
Copy link
Contributor

@vincentorback vincentorback commented Sep 9, 2016

Fix #22

This resolves the issues I was testing in #22 where an undefined variable broke other plugins by providing an undefined variable instead of a variable with the value of "undefined".

Don’t really know what tests to add because the output will be the same in this plugin context. But if you have an idea I could add something! 👍

@MadLittleMods
Copy link
Owner

MadLittleMods commented Sep 10, 2016

@vincentorback Thanks for the PR 😀

The output string will be the same but we can look ast tree and check the type of the value on that node.

var chai = require('chai');
var expect = chai.expect;

var postcss = require('postcss');
var cssvariables = require('postcss-css-variables');

var fs = require('fs');

var mycss = fs.readFileSync('input.css', 'utf8');

describe('postcss-css-variables', function() {
  it('Should use string values for `undefined` values, see #22', function() {
    return postcss([
        cssvariables(/*options*/)
      ])
      .process(mycss)
      .then(function(result) {
        var root = result.root;
        var fooRule = root.nodes[0];
        expect(fooRule.selector).to.equal('.foo');
        var colorDecl = fooRule.nodes[0];
        expect(colorDecl.value).to.be.a('string');
      });
  });
});

}


// Set value to 'undefined' as string if undefined
Copy link
Owner

@MadLittleMods MadLittleMods Sep 10, 2016

Choose a reason for hiding this comment

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

// Set 'undefined' value as a string to avoid making other plugins down the line unhappy
// See #22

@vincentorback
Copy link
Contributor Author

Updated with new undefined check + new comment!
What was the extra whitespace you commented on?

@MadLittleMods
Copy link
Owner

@vincentorback You can expand the past dismissed comments.

Let's add in the test I mentioned earlier.

@vincentorback
Copy link
Contributor Author

I added a test but I’m really new to mocha so take a look if I should change something.

var fooRule = root.nodes[0];
expect(fooRule.selector).to.equal('.box-foo');
var colorDecl = fooRule.nodes[0];
expect(colorDecl.value).to.be.a('string');
Copy link
Owner

Choose a reason for hiding this comment

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

I didn't have this in my snippet but lets also add a expect(colorDecl.value).to.be.equal('undefined');

@MadLittleMods
Copy link
Owner

After fixing up these last points, squash down the commits please

Smaller undefined check

More descriptive comment

Added test for undefined variable to use undefined as string

Moved test and made it async

Fixed undefined value test
@vincentorback
Copy link
Contributor Author

I think that’s it!

@MadLittleMods
Copy link
Owner

@vincentorback Thanks for sorting out the last bits. Will take a look soon 😀

@MadLittleMods MadLittleMods merged commit ad0fc2c into MadLittleMods:master Sep 24, 2016
@MadLittleMods
Copy link
Owner

@vincentorback This has been merged in and released in v0.6.0 and published on npm. Thanks again for the contribution 💞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Displaying readable errors
2 participants