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

[esr360]

Addresses the discussion from #5

[pmowrer]

Thanks for putting some thought into this @esr360 and sorry about the delayed response!

[pmowrer]

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

[esr360]

@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.

[esr360]

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

[pmowrer]

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?

[esr360]

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.

[pmowrer]

Can you elaborate on why using a try/catch here seems crude? It seems like an appropriate use of it to me.

I wasn't commenting on the use of try/catch specifically, but rather on the "appropriate use". It all sounds reasonable to me, but its not clear from looking at the code what cases this is meant to support. Do we know it won't break people? Could we be introducing false positives?

I'm cool with merging this change, but the README needs to be updated to reflect this change (there's a section on strings) and I think shouldBeStringified needs a comment explaining what/why it is doing.

[pmowrer]

Closing due to inactivity