Strings with Spaces or Commas

[lucyllewy]

I have a JSON file which includes values with spaces and commas which cause the parser to fail once the JSON has been imported into SASS. I have therefore forked and fixed this use-case in the referenced branch.

[pmowrer]

Thanks! It would be nice to add a test as well... do you have a good example of the problem?

[lucyllewy]

This is the json file which I had issues with: https://gist.github.com/diddledan/e586a2ed832d25815862

specifically the "alt" items were being translated to unquoted versions of themselves meaning that the spaces and commas became separators rather than part of the value. e.g. the first item became:

alt: Ashness Pier, Derwentwater, Lake District, Cumbria, UK.

so a new variable called "alt" was created and assigned the value "Ashness" and possibly also "Pier" (I'm unclear how spaces are interpreted here) then there's a separator "," which indicates the end of the value and so the next word "Derwentwater" is treated as an additional variable name rather than being part of the value of the variable named "alt".

[pmowrer]

Thanks @diddledan, added a test for this. Your fix is in 1.0.3.

[safareli]

better solution would have been to wrap your strings with quotes inside json

{
    "backgrounds": [
        {
            "filename": "Ashness-Pier-Derwentwater-Lake-District-Cumbria-UK",
            "alt": "\"Ashness Pier, Derwentwater, Lake District, Cumbria, UK.\""
        }
    ]
}```

[lucyllewy]

no, double-quoting would have broken my javascript which uses the same json.

[safareli]

wo that is you have gold in your json?
{color:"gold"}
how should it be loaded then?
as string $color: "gold";
or as is (named color) $color: gold;

or hsl(0,0,0) or string containing quotes

[safareli]

someone also had issue like this but with file path and @pmowrer pointed out solution tho just add inner quotes. the fact that it will break your js code is not problem of this library. in types in json are not direct map of types to sass so best solution would be to just add quotes and don't merge this PR

[pmowrer]

Good points @safareli... I'll reconsider when I get a chance to sit down with this

[safareli]

Great 👍

[pmowrer]

I'm leaning towards reverting c60d310 for the following reasons:

1. There doesn't appear to be a good way of determining whether the user intends to use a JSON value as a string. SASS's ambiguity in allowing strings to be both quoted and unquoted doesn't help.
2. As @safareli has pointed out, wrapping the JSON value in strings introduces all kinds of edge cases. Trying to parse for them all muddies the water and introduces a lot of complexity, especially given:
3. There is an easy way to use strings containing commas/spaces: wrap it in single quotes (or escaped quotes). The downside is it introduces an extra step when parsing the same JSON in JavaScript, but that seems like relatively minor inconvenience.

[safareli]

+1

[safareli]

any updates on this?

[pmowrer]

Reverted in 1.0.4

[safareli]

great 👍

[pyrsmk]

Can it be handled automatically by the lib? In fact, modifying json data just to ensure that sass won't choke is not really good for interoperability. Per example, I have a config file that is used by Sass and PHP.

So, detecting what strings are good and what are not, and then injecting the data in Sass as is should be would be better ^^

[pmowrer]

@pyrsmk We tried to make the lib "handle it" but reverted for the reasons outlined above. The main problem is it's hard to assume what the user intends since SASS is ambiguous in how it handles strings.

That said, if anyone has a proposal for how to handle this by all means go ahead.

[esr360]

This is quite the doozy. Indeed, the point of using something like this library is to have a single source of data in JSON format to use across multiple languages, eg. Sass + PHP or Sass + JS, so I really don't think we should be tweaking our JSON just to bend to Sass's ambiguity.

The downside is it introduces an extra step when parsing the same JSON in JavaScript, but that seems like relatively minor inconvenience.

I disagree completely with this; if I have some JSON data, I shouldn't need an extra step to parse it in JS - if any language here should require an extra step to parse JSON it should be Sass, not JS - that's why we're all using this library in the first place.

I don't really buy into the edge cases highlighted above either. The only real issue I can see that it caused is

in: { "namedColor" : "gold" }
out: $namedColor: "gold";
User of this library may thought to use it as color but it is compiled to string so she can't.

This is a valid point, and perhaps this is an extremely biased opinion, but who even uses color literals in CSS anyway? It's better to use HEX/RGBA/HSL values, this wouldn't be a problem for me or the majority of other developers, I imagine. Especially if the alternative is to force my JSON to become:

{ "namedColor" : "'gold'" }

In Sass we can take a string and remove the quotes to leave the literal color. If we are expecting a value to be an actual color (in Sass) but we are passed a string from JSON we can use built in Sass functions to manipulate it how we like.

It just feels as though the aim here should be to have clean, regular JSON, rather than clean Sass, but I appreciate the complications around this subject.

[pmowrer]

I'd be happy to reintroduce string parsing if someone can come up with a comprehensive proposal for how to handle them (and not just patch individual edge cases).

[esr360]

I will try and come up with some sort of solution. My plan was to fork this project and implement a solution which works for me. Obviously I would much prefer the solution to be implemented into this original repository.

Ultimately, I cannot stress enough that the JSON should absolutely not bend over to accommodate any quirks Sass has; it should 100% be the other way around. Sass can handle anything - if we pass it a string, we can execute the call function on it to dynamically call any functions.

I'm not really experienced enough to come up with a comprehensive proposal here and now, but at the end of the day - the ideal scenario would be to take any occurrences of the error "invalid CSS" and treat it as a string. I don't know how possible this is - but if we have to handle parsing and formatting in Sass ourselves, so be it - my goal is to be able to import valid JSON into a Sass file without it erroring, and I wholeheartedly believe that should be the goal of this project.

[pmowrer]

Ultimately, I cannot stress enough that the JSON should absolutely not bend over to accommodate any quirks Sass has; it should 100% be the other way around.

I think that's fair. Let's try to get this lib to a place where it makes smart assumptions.

[esr360]

I'm thinking on working on a PR for this this week. I've got to the stage where I really need to be able to import valid JSON without double-quoted strings into my Sass.

It might be useful to try and come up with all the edge cases this original PR broke.

Specifically with the HTML color values/names, I see no reason why we couldn't handle this by just comparing the value with a list of HTML colors: https://www.npmjs.com/package/html-colors - naturally if there's a match, it would be treated as a color, otherwise it would be treated as a string.

Yes it's one more dependency, but who cares, even if we have to manually isolate each edge case and put a unique handler in, it's still better than the alternative of not being able to import valid JSON.

Even if we feel like we can't safely come up with all edge cases, I would still rather "brute force" them into the open by making this change and then fixing edge cases as they are reported.

What else is there?

[esr360]

Ok, so I have a project that utilises node-sass-json-importer quite heavily: https://github.com/esr360/One-Nexus/tree/master/src/ui, I probably have over 40 JSON files including all sorts of values: arrays, objects, nested arrays, nested objects, arrays of objects, objects of arrays, boolean values, integers, CSS widths (using %, em, px etc), file paths, strings, you name it. I'd like to think if I can get it working with my project, then most use-cases are probably covered.

It's still early days, but I've made some changes locally which seem to work. I basically worked through each error I was getting until there were no more errors and it compiled.

I've essentially added several else conditions to the parseValue function, trying to make the conditions as general as possible. Here's what I've ended up with (this can probably be refactored):

function parseValue(value) {
  if (_lodash2['default'].isArray(value)) {
    return parseList(value);
  } 
  else if (_lodash2['default'].isPlainObject(value)) {
    return parseMap(value);
  } 
  // Return explicitly an empty string (Sass would otherwise throw an error as the variable is set to nothing)
  else if (value === '') {
    return '""';
  }
  // edge case where `value` is a single special character (apart from, for some reason, an underscore '_')
  else if (typeof value === 'string' && value.length === 1 && /[ !@#$%^&*()+\-=\[\]{};':"\\|,.<>\/?]/.test(value)) {
    return '"' + value + '"';
  }
  // value is a valid CSS selector
  else if (typeof value === 'string' && isValidSelector(value)) {
    return value;
  }
  // value is a number (integer or string, 1 or '1')
  else if (!isNaN(value) && typeof value !== 'boolean') {
    return parseInt(value)  
  } 
  // value contains a number, assume to be CSS value or simple string - "10px" would be output as CSS value, but "alpha5" would be output as a string
  else if (/\d/.test(value)) {
    return value;
  }
  // assume all other strings, such as a file path
  else if (typeof value === 'string') {
    return '"' + value + '"';
  } 
  // everything else, including boolean values
  else {
    return value;
  }
}

One of the conditions above needs to know if the value is a valid CSS selector, so we need a isValidSelector function:

function isValidSelector(selector) {
    var stub = document.createElement('br');
    
    try { 
        stub.querySelector(selector); 
    } catch(e) { 
        return false; 
    }

    return true;
}

Naturally since this is Node.js, there is no document. Which unfortunately means we need to include a library to create one, i.e jsdom. Simply doing the following exposes the document:

require('jsdom-global')();

Now, whilst the isValidSelector function works in the browser, it seems as though jsdom has a bug which I have raised here: jsdom/jsdom#2285, meaning the above function doesn't work with jsdom.

However, using jQuery we can create a working isValidSelector function using jsdom, but again this unfortunately means we need to require jQuery:

var $ = require('jquery');

And we can update the isValidSelector function to:

function isValidSelector(selector) {
    try {
        $(selector);
    } catch(error) {
        return false;
    }
    return true;
}

And now it works. Obviously having to include jsdom and jQuery is far from ideal, but I actually think we definitely need an isValidSelector function to make the feasible. We could probably get rid of jQuery some how though.

As for the parseValue function, we will also need to identify any other edge cases. It probably still needs a lot more testing as I'm probably missing some common things.

[esr360]

So I've already realised that a path with a number will break the compiler with the above, such as assets/images/logo-light-4.png. If we can come up with a good way to determine if a string should be treated as a path, we should be good (or more specifically, a way to determine if a string should not be treated as a path).

I think the above would also convert any CSS value that doesn't contain a number into a string which is obviously not what we want. The more I look the more things the above code doesn't account for...dang. Although there might be hope with https://www.npmjs.com/package/is-valid-css-value. As long as we can predict and account for all edge cases it's worth pursuing in my opinion.

[esr360]

Sorry for spamming this thread...I just get passionate and excited.

I was trying to come up with a way which didn't involve predicting/guessing how a json value should be treated, and I was thinking, couldn't we update the parseValue function to something like:

function parseValue(value) {
  if (_lodash2['default'].isArray(value)) {
    return parseList(value);
  } 
  else if (_lodash2['default'].isPlainObject(value)) {
    return parseMap(value);
  } 
  // Return explicitly an empty string (Sass would otherwise throw an error as the variable is set to nothing)
  else if (value === '') {
    return '""';
  }
  else {
    sass.render({
        data: `$foo: ${value};`
    }, function(error) {
        if (error) {
            value = `"${value}"`;
        }
    });

    return value;
  }
}

We actually pass the value to sass.render and see if it renders...if not we can therefore assume the value must be stringified.

The only issue with the above code is that it doesn't work properly; the function returns before sass.render finishes, so I'm not sure how to get around this without re-structuring the whole thing.

EDIT: Also it seems that node-sass has a few "experimental" features that have been around a while and look quite legit, perhaps they could be made use of some how, not sure...

EDIT2: PROGRESS!

So...I was using the sass.render method when I should have been using sass.renderSync. Now I am able to determine whether a passed value from JSON should be treated as a CSS value or a string by simply attempting to render it with Sass and seeing if it works or not, leaving me with:

export function parseValue(value) {
    if (_.isArray(value)) {
        return parseList(value);
    } 
    else if (_.isPlainObject(value)) {
        return parseMap(value);
    } 
    else if (value === '') {
        return '""'; // Return explicitly an empty string (Sass would otherwise throw an error as the variable is set to nothing)
    } 
    else {
        const result = value => {
            try {
                return sass.renderSync({
                    data: `$foo: ${value};`
                });
            } catch(error) {
                return false
            }
        }

        if (!result(value)) {
            value = `"${value}"`;
        }

        return value;
    }
}

From what I can tell so far it works very nicely. From here, within Sass, if you aren't getting a value as a string and you need it to be one, you can manipulate it very easily to become one.

EDIT 3:

I've updated the code to be more in-line with the rest of the file:

export function parseValue(value) {
  if (_.isArray(value)) {
    return parseList(value);
  } else if (_.isPlainObject(value)) {
    return parseMap(value);
  } else if (shouldBeStringified(value)) {
    return `"${value}"`;
  } else {
    return value;
  }
}

As you can see I was able to remove the condition to check whether value was an empty string, as that is now handled by the new shouldBeStringified function:

export function shouldBeStringified(value) {
  try {
    sass.renderSync({
      data: `$foo: ${value};`
    });

    return false;
  } catch(error) {
    return true
  }
}