Configurable quotes

[jeffrifwald]

The transformers currently return both single (addDisplayName and quoteAttrName) and double quotes (JSON.stringify) in the transformed result, so it is impossible to enforce quote rules. I've added an option called useSingleQuotes that decides if double or single quotes should be used throughout. I may be missing other cases, but this seems to address the issues I have had.

[zpao]

cc @jeffmo

[syranide]

This is not correct, "\\" for instance becomes '\\" if I'm reading it correctly. I could recommend having a look at my previous #1517 which has a very simple and inherently robust implementation (but lacked the CLI option), although it's not as efficient as this could be with the right regexp (but I doubt it's measurable).

[syranide]

You can't just replace the first and last character since the string value '" escapes to '\" (the single quote is not escaped). Your algorithm produces the following when I test it in the browser (string literal quotes are included):

"'\""  =>  ''\"'
"\\"   =>  '\\"

Both are invalid JS.

It's possible to add manual escaping etc, but I think that in the end it will not be measurably faster than the solution in #1517 and since " will remain escaped ('" would become \'\") it in a way actually yields sub-optimal JS. Whereas #1517 is always guaranteed to generate correct and optimal output, as single and double quotes are treated equally by JS.

(Btw, if you and devs agree, feel free to copy it)

[jeffrifwald]

Ah I see what you are saying. I'll go ahead and swap quotes, stringify, then swap back like you proposed unless anyone has any better ideas.

[syranide]

Wouldn't it make more sense to introduce a helper (like quoteAttr) for all stringifying (should be used by quoteAttr as well), rather than implement a slight variation of it for each use.

[jeffrifwald]

Yep, will do for this and quoteAttr. I am uncertain if it makes sense to introduce JSON.stringify for these values, so these two cases will be kept separate from xjs.js line 193-195 unless we decide all 3 values should be passed through JSON.stringify.

[syranide]

@jmcriffey I think since they're all about creating stringified representations with single/double quotes, it makes sense to centralize the stringifying unless it measurably impacts performance (which I sincerely doubt).

It would also mean that all operations are inherently safe, they kind of are already because of the way we parse displayName and attributes, but that is not necessarily guaranteed (especially since attribute names can technically have '" in them).

[jeffrifwald]

@syranide That makes total sense. I'd like to get some more feedback. I assume escaping the displayName and attrName was either overlooked or there was a good reason to omit it, as of now single quotes are used on the outside but nothing seems to be escaped in the value. Either way, it should be consistent. I am sure someone has opinions on why or why not displayName and attrName should be run through JSON.stringify.

[syranide]

@jmcriffey I would assume it was simply "overlooked", or rather because "it works". @jeffmo could probably shine some light on the subject.

cc @jeffmo

[syranide]

They maintain an 80 char limit

utils.append(
  'displayName: ' + doubleOrSingleQuotes(displayName, state) + ",",
  state
);

Also, inconsistent use of ' and ", we use ' everywhere it makes sense (IIRC the JSX source is a bit inconsistent at the moment). It may make sense to use " instead if there's a ' that would otherwise have to be escaped.

[jeffrifwald]

I'll match the closest code to whatever I'm working in. Looks like ' in all cases. If only there was a magical way to enforce quoting.

[zpao]

@jeffmo, where you at? I'm inclined to say "no" and maybe it can be better in a recast world.

[constantx]

would love to have this option as well :)

any update?

[sebmarkbage]

We do want to allow this feature. However, we want to switch to Recast for the transform. So we would require that this feature was available in Recast before we added it.

[amsul]

@sebmarkbage well it seems now recast does support this feature. Do you think we could get this option added in now? :)