-
Notifications
You must be signed in to change notification settings - Fork 55
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
Strings with Spaces or Commas #5
Strings with Spaces or Commas #5
Conversation
Added support for quoting string values from the JSON when they contain spaces or commas.
Thanks! It would be nice to add a test as well... do you have a good example of the problem? |
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:
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". |
Thanks @diddledan, added a test for this. Your fix is in |
better solution would have been to wrap your strings with quotes inside json
|
no, double-quoting would have broken my javascript which uses the same json. |
wo that is you have or |
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 |
Good points @safareli... I'll reconsider when I get a chance to sit down with this |
Great 👍 |
I'm leaning towards reverting c60d310 for the following reasons:
|
+1 |
any updates on this? |
Reverted in |
great 👍 |
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 ^^ |
@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. |
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.
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
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:
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. |
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). |
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 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. |
I think that's fair. Let's try to get this lib to a place where it makes smart assumptions. |
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? |
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 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 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 require('jsdom-global')(); Now, whilst the However, using jQuery we can create a working var $ = require('jquery'); And we can update the function isValidSelector(selector) {
try {
$(selector);
} catch(error) {
return false;
}
return true;
} And now it works. Obviously having to include As for the |
So I've already realised that a path with a number will break the compiler with the above, such as 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. |
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 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 The only issue with the above code is that it doesn't work properly; the function returns before 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 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 export function shouldBeStringified(value) {
try {
sass.renderSync({
data: `$foo: ${value};`
});
return false;
} catch(error) {
return true
}
} |
Currently we coerce all values into quoted Sass Strings. This PR coerces the JSON value into the appropriate Sass value type. If the value type cannot be determined we fallback to a quoted Sass String to avoid errors. This differs from [prior work][1] in that no attempt is made to mangle JSON arrays and maps into a JSON string value. [1]: pmowrer/node-sass-json-importer#5 Fixes #3
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.