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.

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

Jump to bottom

determining if value should be treated as a CSS value or a string #70

Closed
wants to merge 4 commits into from
Closed

determining if value should be treated as a CSS value or a string #70

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f264b38
determining if value should be treated as a CSS value or a string
esr360 Jul 5, 2018
bd6973f
updating var to const
esr360 Jul 5, 2018
2ab42f9
making code intention clearer
esr360 Jul 6, 2018
433abf0
adding shouldBeStringified function and removing superfluous condition
esr360 Jul 6, 2018
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
17 changes: 15 additions & 2 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import _ from 'lodash';
import isThere from 'is-there';
import sass from 'node-sass';
import path, {resolve, basename, extname} from 'path';

import 'json5/lib/register'; // Enable JSON5 support
Expand Down Expand Up @@ -61,8 +62,8 @@ export function parseValue(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 if (shouldBeStringified(value)) {
return `"${value}"`;
} else {
return value;
}
Expand All @@ -81,6 +82,18 @@ export function parseMap(map) {
.join(',')})`;
}

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

Choose a reason for hiding this comment

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

What's $foo? I think this needs some explanation... what's the idea behind shouldBeStringified? What are the criteria?

Copy link
Author

@esr360 esr360 Jul 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@pmowrer if you haven't already please read my comments starting from here: #5 (comment) - you can follow the process that led me to this solution.

The idea behind the shouldBeStringified function is that it attempts to determine whether a value passed to parseValue should treated as a string and returned as "${value}".

$foo is just a placeholder variable we attempt to assign the non-stringified value to. If this breaks the Sass compiler, we know that Sass needs it to be a string to compile, hence we return shouldBeStringified as true.

Been using this branch on my side project since opening it and I've had no issues - and I have a plethora of various JSON values of different types.

I'm happy to make whatever improvements you need.

Copy link
Author

Choose a reason for hiding this comment

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

@pmowrer it's probably also worth you checking this out sasstools/json-importer#3

Copy link
Owner

Choose a reason for hiding this comment

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

It seems a bit crude to just do a try/catch... do we know it's backwards compatible? Would you mind summarizing your reasoning from your discussions in these other tickets?

Copy link
Author

@esr360 esr360 Jul 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Can you elaborate on why using a try/catch here seems crude? It seems like an appropriate use of it to me. The only way we can know for sure if a value will compile in Sass is to simply "try" it, and "catch" whether it errors or not; that's what makes this solution so simple. We don't have to try and preempt whether a value should be returned as "${value}" based off some logic, we can literally just try it, and catch the error before it actually happens, and then manipulate the value when needed.

As far as compatibility is concerned, I am not aware of any compatibility issues with try/catch, at least for basic usage such as this case.

I can't really summarize any more than I already have:

The idea behind the shouldBeStringified function is that it attempts to determine whether a value passed to parseValue should treated as a string and returned as "${value}".

$foo is just a placeholder variable we attempt to assign the non-stringified value to. If this breaks the Sass compiler, we know that Sass needs it to be a string to compile, hence we return shouldBeStringified as true.

@xyfer said (in this thread sasstools/json-importer#3) "The inefficiency is executing Node Sass for each value in shouldBeStringified. What I was suggesting is I think there is why to achieve the same behaviour without this overhead."

So whilst the implementation of the solution may not be as efficient as it could be, the solution itself seems good, and I can't say I've noticed any significant time overhead since using it.

To be clear, my experience from this solution is that I'm able to import any valid JSON file into my Sass without it ever erroring and without any unexpected behaviour in my Sass, and without having to format my JSON in a special way.

});

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

// Super-hacky: Override Babel's transpiled export to provide both
// a default CommonJS export and named exports.
// Fixes: https://github.com/Updater/node-sass-json-importer/issues/32
Expand Down