Add an option to modify when Prettier quotes object properties by azz · Pull Request #5934 · prettier/prettier

azz · 2019-03-02T02:35:22Z

This PR introduces a new option to Prettier:

`--quote-props <as-needed|preserve|consistent>`

as-needed (default) - Only add quotes around object properties where required. Current behaviour.
preserve - Respect the input (#4327)
consistent - If at least one property in an object requires quotes, quote all properties (#838) - this is like ESLint's consistent-as-needed option

Rationale

The primary motivation to add an option was to unblock users of the Google Closure Compiler's advanced mode. Removing quotes was actually breaking code, so it was a complete blocker to adopting Prettier. This is detailed in #4327.

While we were adding this option it made sense to also support a popular preference of quoting all keys in an object if one key requires it. e.g.:

// --quote-props=as-needed
const headers = {
  accept: 'application/json',
  'content-type': 'application/json',
};

// --quote-props=consistent
const headers = {
  'accept': 'application/json',
  'content-type': 'application/json',
};

Fixes #838
Fixes #4327

I’ve added tests to confirm my change works.
(If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
(If not an internal change) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
I’ve read the contributing guidelines.

✨Try the playground for this PR✨

azz · 2019-03-02T02:38:42Z

src/language-js/printer-estree.js

+    parent.properties ||
+    parent.body ||
+    parent.members
+  ).some(


This .some() call causes printPropertyKey to be O(n^2) in number of props because for each property being printed we look at all other properties, this may cause perf issues for very large objects. Thinking of memoizing the result with mem. Thoughts?

(We also only need to compute this if quoteProps === 'consistent' but I left it like this during development so all the tests hit this code path.)

You could do this check in the parent node. I’m not sure in how many contexts object keys can appear but hopefully it’s a small set.

There are 6 calls to this function, but they're called from property levels of the AST, so I'd need to go one level up the AST again to get the parent node, difficult to figure out all the possible cases, and if we did we'd have to mutate the AST to store the info required.

What is the mem thing you are talking about, I wasn't aware that we had a way to memoize computation within prettier infra. If we have it, then that would be a great fit.

If it doesn't exist, we can mutate the object for now, it's not ideal but not the end of the world either. I don't think that we're able to reuse ast right now.

mem is a memoization library, we use it to cache configuration with cosmiconfig.

Thinking more though, we could probably just use a WeakMap where the key is the parent AST node, and the value is whether the object/class/whatever has quoted properties. We'd just create a fresh map for every top-level print call.

Just implemented WeakMap. I took the simplest approach of using one module-local WeakMap instance, and never clearing it. If my understanding of WeakMap is correct, we should never have to clear it, because as soon as the AST goes out of scope, the keys will get garbage collected and automatically removed from the map. If anyone more experienced with WeakMap thinks otherwise, let me know. 👍

src/language-js/printer-estree.js

kachkaev · 2019-03-02T10:33:51Z

Could this option be removed in v2 in favour of --quote-props consistent at all times?

azz · 2019-03-02T11:33:31Z

It won't be removed, we still need preserve, but changing the default to consistent in V2 would get my 👍

tests/quote_props_typescript/__snapshots__/jsfmt.spec.js.snap

kachkaev · 2019-03-02T17:08:45Z

@azz what about just making preserve + consistent as a default behaviour in v2 and thus still avoid the option? If at least one key is already quoted, all become quoted too. This would be similar to what's done to the object formatting already: if it can fit one line and there is no \n after {, Prettier tries to squeeze it in.

Just trying to play the role of the option philosophy advocate 😉

UPD. Ah, I misunderstood preserve originally. Why do you think we need this option in v2?

lydell · 2019-03-02T17:26:41Z

git diff?

kachkaev · 2019-03-02T17:33:02Z

🤔 I thought v2 would be somewhat git-diff heavy by definition. E.g. if #3806 is implemented without becoming an option.

lydell · 2019-03-02T17:40:39Z

I thought v2 would be somewhat git-diff heavy by definition.

That's not the vibes I've been picking up.

azz · 2019-03-03T00:24:42Z

We need preserve for Closure Compiler users.

console.log({
  'foo': 'foo',
  baz: 'baz'
});

// compiles to this:

console.log({foo:"foo",a:"baz"});

vjeux · 2019-03-03T19:09:14Z

I want to make sure that the O(n^2) behavior is resolved and we don't run the computation unless the proper option is passed. When that's addressed, I'm happy to merge. Thanks @azz for working on it so quickly! It's going to unlock the usage of prettier for all the people using Google Closure Advanced Mode!

mgol · 2019-03-03T22:42:48Z

It won't be removed, we still need preserve, but changing the default to consistent in V2 would get my 👍

Please don't change the default to one that will generate huge diffs for small changes for some projects. My thoughts on that: #838 (comment)

vjeux

Great, the WeakMap solution is very elegant and should work well :)

suchipi · 2019-03-06T11:40:24Z

🎊🎉

lipis · 2019-03-06T14:42:26Z

Maybe we could have a release soon, because of this option.

duailibe · 2019-03-06T15:18:18Z

@kachkaev Just to make it clear, the preserve behavior is the primary reason we're adding this option, and while we're at it, we might as well add consistent too.

If it wasn't for the Google Closure Compiler issue, we probably wouldn't add this option at all.

rsimha · 2019-03-14T22:12:24Z

@azz @vjeux Thanks for this much-awaited fix! Any idea when it will be released so we can try it out?

(Context: we're eagerly looking to use prettier with our closure-compiled AMP project, and are currently blocked by ~~#1013~~, #4327, #4799, #5588, and #5973)

j-f1 · 2019-03-14T22:33:55Z

You can use npm install prettier/prettier or yarn add prettier/prettier to install the version from GitHub, but be aware that it pulls in a bunch of dependencies.

rsimha · 2019-03-15T00:35:15Z

Ideally, we'd like to use a released version, since we lock down versions in our package.json. Happy to wait until this and other bugs are fixed, and a new release is pushed out. Thanks for the tip, though!

medikoo · 2019-04-02T11:15:32Z

It's great to have it. Still as I look ES3 compatibility is still not preserved with Prettier

e.g. { "default": "whatever" } is always translated to { default: "whatever" }

ESLint for that provides additional keywords: true option for quote-props rule.

It'll be good to have an option to write ES3 compliant code with Prettier

lydell · 2019-04-02T14:33:19Z

@medikoo This feature has not been released yet.

medikoo · 2019-04-02T18:37:47Z

@medikoo This feature has not been released yet.

@lydell I know. Whether it was released yet or not is irrelevant, that's not my point.

lydell · 2019-04-02T18:39:31Z

Then what is your point?

medikoo · 2019-04-02T18:49:38Z

Then what is your point?

I think my comment was quite clear, still let me put same message with other words:

I'm happy this feature landed in master (although it's not yet released), still it looks incomplete, as Prettier still produces ES3 incompliant output for ES3 compliant input.

lydell · 2019-04-02T19:27:43Z

Thanks. I didn't see before that most of the second line was a link (to the playground for this PR).

I think that ES3 compliance is really niche. If you really need that, I'd say go with --quote-props preserve + ESLint.

medikoo · 2019-04-02T19:29:59Z

I'd say go with --quote-props preserve + ESLint.

Indeed that should work, thanks

azz added 4 commits March 2, 2019 11:35

Implement quote-props=preserve

769b5a5

Implement quote-props=consistent

31448d8

Add tests

de734f5

Enable quoteProps in playground

639ea6c

azz requested review from suchipi and vjeux March 2, 2019 02:35

azz commented Mar 2, 2019

View reviewed changes

src/language-js/printer-estree.js Outdated Show resolved Hide resolved

azz commented Mar 2, 2019

View reviewed changes

src/language-js/printer-estree.js Show resolved Hide resolved

azz mentioned this pull request Mar 2, 2019

Respect quoted object keys, as unquoting breaks aggressive minifiers like Closure Compiler #4327

Closed

suchipi reviewed Mar 2, 2019

View reviewed changes

tests/quote_props_typescript/__snapshots__/jsfmt.spec.js.snap Show resolved Hide resolved

Add documentation

ac1ea45

Cache objectHasStringProp calculations

4f9e49a

azz marked this pull request as ready for review March 4, 2019 11:11

Fixup changelog

73d482c

azz force-pushed the quote-props branch from a741052 to 73d482c Compare March 4, 2019 11:29

vjeux approved these changes Mar 4, 2019

View reviewed changes

lipis approved these changes Mar 5, 2019

View reviewed changes

azz merged commit f526c47 into prettier:master Mar 6, 2019

azz deleted the quote-props branch March 6, 2019 09:33

lipis added this to the 1.17 milestone Mar 15, 2019

wincent mentioned this pull request Mar 22, 2019

Change quote-props rule to "consistent-as-needed" liferay/eslint-config-liferay#39

Open

GerHobbelt mentioned this pull request Mar 24, 2019

prettier produces broken code for quoted object properties: latest Chrome browser b0rks on Unicode accents/intricacies (March 2019) #6007

Closed

lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 2, 2019

lock bot locked as resolved and limited conversation to collaborators Jul 2, 2019

Uh oh!

Conversation

azz commented Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

--quote-props <as-needed|preserve|consistent>

Rationale

Uh oh!

azz Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

azz Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

vjeux Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

azz Mar 2, 2019

Choose a reason for hiding this comment

Uh oh!

vjeux Mar 3, 2019

Choose a reason for hiding this comment

Uh oh!

azz Mar 4, 2019

Choose a reason for hiding this comment

Uh oh!

azz Mar 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kachkaev commented Mar 2, 2019

Uh oh!

azz commented Mar 2, 2019

Uh oh!

Uh oh!

kachkaev commented Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lydell commented Mar 2, 2019

Uh oh!

kachkaev commented Mar 2, 2019

Uh oh!

lydell commented Mar 2, 2019

Uh oh!

azz commented Mar 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vjeux commented Mar 3, 2019

Uh oh!

mgol commented Mar 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vjeux left a comment

Choose a reason for hiding this comment

Uh oh!

suchipi commented Mar 6, 2019

Uh oh!

lipis commented Mar 6, 2019

Uh oh!

duailibe commented Mar 6, 2019

Uh oh!

rsimha commented Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-f1 commented Mar 14, 2019

Uh oh!

rsimha commented Mar 15, 2019

Uh oh!

medikoo commented Apr 2, 2019

Uh oh!

lydell commented Apr 2, 2019

Uh oh!

medikoo commented Apr 2, 2019

Uh oh!

lydell commented Apr 2, 2019

Uh oh!

medikoo commented Apr 2, 2019

Uh oh!

lydell commented Apr 2, 2019

Uh oh!

azz commented Mar 2, 2019 •

edited

Loading

`--quote-props <as-needed|preserve|consistent>`

kachkaev commented Mar 2, 2019 •

edited

Loading

azz commented Mar 3, 2019 •

edited

Loading

mgol commented Mar 3, 2019 •

edited

Loading

rsimha commented Mar 14, 2019 •

edited

Loading