Skip to content

Add escapeStrings option and tests #40

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Add escapeStrings option and tests #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,15 @@ import _ from 'lodash';
import {resolve} from 'path';
import isThere from 'is-there';

var ESCAPE_STRINGS = false;

export default function(url, prev) {
if (!isJSONfile(url)) {
return null;
}

ESCAPE_STRINGS = this.options.escapeStrings;

let includePaths = this.options.includePaths ? this.options.includePaths.split(':') : [];
let paths = []
.concat(prev.slice(0, prev.lastIndexOf('/')))
Expand Down Expand Up @@ -50,6 +54,8 @@ export function parseValue(value) {
return parseList(value);
} else if (_.isPlainObject(value)) {
return parseMap(value);
} else if (ESCAPE_STRINGS && _.isString(value)) {
Copy link
Owner

@pmowrer pmowrer Jan 5, 2017

Choose a reason for hiding this comment

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

All caps as in ESCAPE_STRINGS usually denotes a constant, which ESCAPE_STRINGS isn't in this case. IMO, it'd be clearer (and simpler) to just use this.options.escapeStrings directly.

Copy link
Author

@berstend berstend Jan 10, 2017

Choose a reason for hiding this comment

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

Ah, while updating my PR I've realized why I've used the variable as intermediate: this is undefined in parseValue.

Copy link
Author

@berstend berstend Jan 10, 2017

Choose a reason for hiding this comment

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

Is there a better way than to put e.g. var options; in the outside scope and use that in parseValue? 😄

Copy link
Owner

@pmowrer pmowrer Jan 12, 2017

Choose a reason for hiding this comment

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

Great point! I think the option should be passed to parseValue to keep that function pure.

export function parseValue(value, { escapeStrings = false }) {
  ...

return `"${value}"`
} else {
return value;
}
Expand Down
26 changes: 26 additions & 0 deletions test/fixtures/escape-strings/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
@import 'variables.json';

/// Map deep get
/// @author Hugo Giraudel
/// @access public
/// @param {Map} $map - Map
/// @param {Arglist} $keys - Key chain
/// @return {*} - Desired value
@function map-deep-get($map, $keys...) {
Copy link
Owner

@pmowrer pmowrer Jan 5, 2017

Choose a reason for hiding this comment

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

I like the real-world test, but all of this SASS logic kind of obscures what's actually being tested. I think we should be able to simplify this a great deal to really get at what it is that we want to test. Review #5 for some simpler examples, like @diddledan's original example or @safareli's list of edge cases.

@each $key in $keys {
$map: map-get($map, $key);
}
@return $map;
}

// Fetch nested data from JSON
$entry-name: map-get($sony, name);
$entry-selectors: map-deep-get($sony, "selectors", "entry");

// Generate CSS based on this data
@each $entry in $entry-selectors {
#{$entry}:before {
color: red;
content: "#{$entry-name}";
}
}
34 changes: 34 additions & 0 deletions test/fixtures/escape-strings/variables.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
{
"google": {
"name": "Google",
"patterns": ["*//*.google.*/*"],
"selectors": {
"entry": ["#google > div"],
"anchor": "h2 a"
}
},
"microsoft": {
"name": "Microsoft",
"patterns": ["*//*.subdomain.microsoft.com/*", "*//microsoft.com/*"],
"selectors": {
"entry": ["#main > li"],
"anchor": "h3.title a"
}
},
"sony": {
"name": "sony",
"patterns": ["*//www.sony.com/"],
"selectors": {
"entry": ["#results > li", "#results2 > li"],
"anchor": "h2 > a"
}
},
"apple": {
"name": "Apple",
"patterns": ["*//apple.com/"],
"selectors": {
"entry": ["#links > .result"],
"anchor": "h2 > a"
}
}
}
31 changes: 31 additions & 0 deletions test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,37 @@ describe('Import type test', function() {
expect(result.css.toString()).to.eql(EXPECTATION);
});

it('escapes strings when escapeStrings option is set', function() {
Copy link
Owner

@pmowrer pmowrer Jan 5, 2017

Choose a reason for hiding this comment

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

When the word "when" is used in the test description, it's usually a sign that part of it can be moved into a describe as grouping mechanism. It allows for adding multiple tests for the same condition. E.g.:

describe('when escapeStrings options is set', function() {
  it('escapes string values', function() {
     ...
  });

  it('does something else', function() {
    ...
  });
});

const escapeStringsExpectation = `#results > li:before {
color: red;
content: "sony"; }

#results2 > li:before {
color: red;
content: "sony"; }
`
let result = sass.renderSync({
file: './test/fixtures/escape-strings/style.scss',
escapeStrings: true,
importer: jsonImporter
});

expect(result.css.toString()).to.eql(escapeStringsExpectation);
});

it(`throws on complex JSON when escapeStrings option is not set`, function() {
Copy link
Owner

@pmowrer pmowrer Jan 5, 2017

Choose a reason for hiding this comment

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

I think "complex JSON" isn't specific enough. You could construct complex JSON that doesn't throw in this test. Something like "throws when parsing a string that isn't a valid SASS expression".

function render() {
sass.renderSync({
file: './test/fixtures/escape-strings/style.scss',
importer: jsonImporter
});
}

expect(render).to.throw(
'Invalid CSS after "...gle,patterns: (": expected expression (e.g. 1px, bold), was "*//*.google.*/*),se"'
);
});

it('finds imports via includePaths', function() {
let result = sass.renderSync({
file: './test/fixtures/include-paths/style.scss',
Expand Down