Allow Object Literal to be Explicit with String Names

[sebmarkbage]

var foo = {
  'bar': 123
};
window.bar = function() {
  console.log(foo['bar']);
}

Turns into:

var foo = {
  bar: 123
};
window.bar = function() {
  console.log(foo['bar']);
}

Note that the object literal lost its explicit string encoding. This breaks Google Closure Compiler's advanced mode heuristic/convention around property name mangling. Uglify has this mode too.

Maybe an explicit mode would be good for this? Or should it be default?

[bakkot]

Uuuugh. I really dislike the idea of making formatting decisions based on tools which treat formatting information as semantic. Do those tools not have a way of providing that information out-of-band from the AST, say with a comment?

[sebmarkbage]

Anyone using those tools (like React is planning on doing) wouldn't really like that noise since it's all over the place.

[yamafaktory]

@sebmarkbage this should be fixed with #1108. @bakkot I don't see keeping object keys as strings (when they originaly were) as a formatting decision, IMHO those should be kept.

[bakkot]

I don't see keeping object keys as strings (when they originaly were) as a formatting decision

This statement confuses me.

({a:0}) and ({'a':0}) are semantically identical. Engines parse them to the same internal representation. The presence or absence of quotes is no more significant than whitespace. How could it be anything other than a formatting decision?

[yamafaktory]

@bakkot Agree, but if we change 'a' to be a then this is a formatting decision and I don't think Prettier should modify it.

[bakkot]

@yamafaktory Isn't the point of prettier to make your formatting decisions for you? It's a formatter. That's what it does.

[yamafaktory]

@bakkot I partially agree. Prettier is opinionated but in this case, I think this is somehow different than just indenting a piece of code in a different way or removing unnecessary parenthesis. Keeping the original quotes can also help if as a team you want to be consistent in the way you declare an object (let's say in big config file), e.g. https://prettier.github.io/prettier/#%7B%22content%22%3A%22var%20a%20%3D%20%7B%5Cn%20%20%5C%22prop%20as%20string%5C%22%3A%201%2C%5Cn%20%20%5C%22propA%5C%22%3A%202%2C%5Cn%20%20%5C%22propB%5C%22%3A%203%5Cn%7D%22%2C%22options%22%3A%7B%22printWidth%22%3A80%2C%22tabWidth%22%3A2%2C%22singleQuote%22%3Afalse%2C%22trailingComma%22%3A%22%22%2C%22bracketSpacing%22%3Atrue%2C%22jsxBracketSameLine%22%3Afalse%2C%22parser%22%3A%22%22%2C%22doc%22%3Afalse%7D%7D

I will let @vjeux / @jlongster decide on that.

[bakkot]

Also, context: #90, #838

[SimenB]

I know there's an aversion to options, but I'd like to see a subset of the options eslint provides: always, as-needed, and consistent-as-needed. Meaning it either always puts it in, it just puts it in where it's syntactically needed, or or quotes all properties if any is needed. There are more options. Missing is consistent, but I don't think that's needed as an option.

The advantage is that if you always want to quote it (like this issue) it's possible, but if you want to be quote-less, that's also a possibility. I threw in consistent-as-needed as well (which is what I use (used I suppose since prettier)) as I find it more readable to not mix quoted and unquoted props, but I can see the argument that it's not needed.

Source: http://eslint.org/docs/rules/quote-props

[vjeux]

@SimenB in this case none of those options would work, Google Closure Advanced mode gives a meaning to the presence or absence of quotes, so we cannot change them without changing the meaning of the program.

[SimenB]

Oh, didn't know that. A 4th. option, "don't touch" then 😁

Although I suppose one might be left with just the current (as-needed, I suppose) and don't touch, which might be enough 😄

[jlongster]

The google closure use case is a pretty strong one. I think we should respect the original style of object keys. I can't imagine many people fight over whether or not keys should be as strings or not, so I don't think we're solving much of a pain point by being opinionated about it, and only make it harder for tools like closure.

While you can infer that { 'foo': bar } is the same as { foo: bar }, I don't think it's too far to think of the string as more of an expression like { ['foo']: bar }. Basically I don't think closure is in the wrong to work the way it does. For it's advanced mode, it already is very restrictive about what's allowed and you can't do a lot that you normally could.

[bakkot]

@jlongster, re:

I can't imagine many people fight over whether or not keys should be as strings or not

This is exactly the sort of formatting issue which I want a tool like prettier to handle. (That's why I implemented the current behavior in the first place!) It would be easy enough to add yet another eslint --fix to get around prettier not doing this, but it would make me sad.

[jlongster]

I agree that this is the sort of stuff I'd like prettier to normalize, but when it comes at the cost of something powerful like Closure's advanced mode (which is really amazing), I think we should back off on it. When there are places that we can make these decisions without affecting anything else very much, I'm all for it, but we have to also recognize the existence of other tools and how they work to some extent.

[SimenB]

Prettier should work to make code readable and consistent for humans, not tools. Can't people needing the quotes just add // prettier-ignore? Or if prettier is changed, at least behind an option, I don't want to re-add eslint rules I removed when I started using prettier

[bakkot]

Hm. Does the Closure compiler compile ({ ['foo']: bar }) to ({ 'foo': bar })? I believe it does, in which case people who want to signal that information to Closure could use ['foo'] instead of 'foo' without too much noise, and prettier would leave it alone. It's more explicit anyway.

@sebmarkbage thoughts? Would that work for your team?

[stringham]

Changing all of our Closure annotated JavaScript to use {['foo']: bar} would be very prohibitive. There wouldn't be an easy way to apply a change like that throughout our large code base.

It looks like the closure compiler doesn't treat them the same:

console.log({['foo']:4}) outputs var a={};console.log((a.foo=4,a)); (link)

where console.log({'foo':4}) outputs console.log({foo:4}); (link)

and console.log({foo:4}) outputs console.log({a:4}); (link)

adding a // prettier-ignore comment to all of the places we need to work with data from the server would also not be feasible.

I appreciate @jlongster's insight on recognizing the existence of other tools and how they work to some extent.

[bakkot]

It looks like this has the effect you want - property names don't get mangled - but it's a shame it doesn't treat them exactly the same. Maybe file a bug with them?

---

There wouldn't be an easy way to apply a change like that throughout our large code base.

Sure there is; just write a codemod. It's super short:

module.exports = function () {
  return {
    visitor: {
      ObjectProperty(path, state) {
        let node = path.node;
        if (node.key.type !== 'StringLiteral' || node.computed) return;
        node.computed = true;
      }
    }
  };
}

Save that as fix-keys.js, then just codemod --plugin ./fix-keys.js src/, where codemod is babel-codemod. I'm sure it's also straightforward with jscodeshift, if you already have your tooling set up for it instead.

[vjeux]

In practice, I haven't seen people successfully adopting the closure compiler with advanced mode outside of Google internal codebase and only a single person using it raised an issue about it. So, given this lack of usage, I'm going to close this issue for now. If React actually manages to do it, I may reconsider.

[emirotin]

Let me bump this one with another use-case. It's a matter of visual style for me, but the changes prettier makes make the code harder to read.

For context I'm working on some utility that tests the text for the specific patterns.
Here's a screenshot of my tests file:

Now if you look at the hashes defining the tests and the expected results, you can see that PT, y and x7 keys they're unquoted and thus are not highlighted as strings. Which makes it harder to scan the text.

[kentor]

same thing with classnames in jsx:

<Something
  className={cx({
    'disabled': true,
    'btn-primary': true,
  })}
/>

would not want the disabled to lose its quotes

[IslamWahid]

there must be an option and let me choose whether to remove or keep the quotes