prettier: Respect quoted object keys, as unquoting breaks aggressive minifiers like Closure Compiler

PR is up at #5934 - Feedback welcome!

I’m thinking we can solve this, as well as #838 with a new option:

--quote-props

Values:

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

@duailibe the options on the Prettier side I can think of are:

• Keep string names where they are found (pretty simple, see example implementation in #1108)

• Work in a consistent way instead of consistent-as-needed (https://eslint.org/docs/rules/quote-props#options), i.e. if Prettier finds at least one quoted key, then quote all keys.

If we wanted a zero-config approach, the least-surprising behaviour to me would be the first.

If we’re ok with adding an option, then the second (i.e. a quoteProps option to match eslint/tslint) seems the most appealing. At the moment Prettier forces people into the consistent-as-needed lint bucket.

In addition to the Closure/mangler needs here, the aesthetic issue raised in https://github.com/prettier/prettier/issues/1103#issuecomment-330839212 is a real thing - I’m surprised more people haven’t run into that!

For what it’s worth, within Google we have discussed adopting prettier but (obviously) as the main users of Closure compiler, this bug means we cannot. I also expect converting all our existing code to put brackets around quoted object properties is a non-starter.

I appreciate the rationale given on these bugs, and it’s not your responsibility to solve our problems, but just in case you wanted another use case. In any case clang-format works well enough for us.

It’s a shame that people using Google Closure Compiler Advanced Mode can’t use prettier. The intention behind having few options is to prevent bike-shedding, not to prevent people from using the project altogether.

Let’s discuss and find a solution that would solve this problem. I know that @azz has some ideas.

@evmar Personally, from afar I think the “use computed properties if you want closure not to mangle them” rule is much easier to learn than the “use quoted property names if you want closure not to mangle them” rule you’re presumably currently teaching, especially given that there are other reasons to use quoted property names (i.e., non-identifier keys). And then non-mangled keys stand out a lot more: I think that, for myself, I would be much more likely to see { ['some-key']: val } and think to myself “oh, that’s not going to get mangled, I should change the key if I want it to be” than I would be for { 'some-key': val }. Especially if my editor changed my 'some-key' to ['some-key'] on save.

As such, I’d think that long-term it would be better to switch to computed keys anyway, even if it does come with some transitional cost.

Okay, so it sounds like there’s actually a reasonable workaround for Closure Compiler with advanced mode: putting brackets around the key: {['key']: value}. While it is not optimal, I believe that it is the best way forward for a few reasons:

• Closure Compiler with Advanced Mode is barely used in practice. In all the existence of prettier, you are only the second person to say that they are actually using it.
• The workaround of adding [] around keys is not pretty but is not unreasonable either and I would argue that it is more explicit than just having quotes changing the meaning of the key.
• My understanding is that quoted keys should be the exception rather than the norm, so it shouldn’t affect most of your codebase (I could be wrong though, if you could get some stats on this from your codebase that would be very helpful).
• If we can get away with not adding an option for this, I believe that it would be better for the future of prettier.

Let me know what your thoughts are.

This singular issue prevents my team from using Prettier. We chose Closure Compiler because it consistently optimizes the best out of any tool in our tests. Without being able to preserve object literals which we painstakingly limit for only certain use cases in our build, Prettier is a non starter. Its a shame because the project is pretty awesome!

@vjeux The biggest hurdle, and unfortunately one outside of your control, is that clang-format works decently well already and is integrated into lots of our other layers due to it covering C++/Java. So the main benefit for adopting prettier for us would just be synchronizing with best practices “outside”. This is definitely something we care about, but it’s hard to justify the work of doing it.

@evmar: I’d love to read that document if you’d be willing to share it. Maybe we can work on finding a middle ground such that you can use prettier at Google

The surface of this is more than just object literals, in that e.g. obj.hasOwnProperty() takes a string and not a computed property. So the rule is still about being careful where you use strings, with the addition of a new syntax when those strings show up in object literals.

I appreciate that you don’t want to budge on this. I am just creating a mini-doc internally about why we hadn’t adopted prettier and wanted to make sure I covered all the points. Here, it’s not “cannot quote object properties” (which is what the bug title suggests at a glance), but rather “would need to change our JS style, linter, and education around object properties”. It’s not surmountable for sure, but also not an obvious improvement. Thanks for thinking it through with me!

@evmar, can I ask why converting existing code to put brackets around quoted objects is a non-starter? You wouldn’t need to do it for all code, just for projects which are trying to adopt prettier, and it can be done automatically with little effort. I appreciate wanting to avoid that sort of churn in general, but doing it as part of the process of adopting prettier wouldn’t create much additional churn over just introducing prettier in the first place, I’d think.

I don’t think we need to leave this open since it’s not actionable. Comment if anyone disagrees and we’ll reopen.

Once the Closure issue gets fixed, I think we can close this issue as there will be no runtime issues.

@alextreppass:

It actually produces different, far-less-optimal output:

Seconding @duailibe that this is a bug in the closure compiler; if this bug with them is the only blocker, I don’t think prettier should be working around bugs in other tools. Perhaps the best way forward is to raise this with them? Probably worth doing so regardless of how prettier proceeds, honestly.

Whereas before string keys would be kept, this time we’d need to make sure the devs do things like include a // prettier-ignore above the certain statements going forward, which may be hard to enforce with tooling.

Really? I would expect this to be almost trivial to write an eslint plugin for. A sketch:

module.exports = {
  meta: { /* the usual boilerplate */ },
  create: function(context) {
    return {
      Property: function(node) {
        if (node.computed || node.key == null || node.key.type !== 'Literal') {
          return;
        }
        let comments = context.getSourceCode().getCommentsBefore(node);
        if (!comments.some(c => c.value === ' prettier-ignore')) { // TODO use whatever check prettier does, not this simple matching
          context.report({
            node,
            message: 'String keys should have prettier-ignore comments, for compat with closure compiler',
          });
        }
      },
    };
  }
};