Skip to content

Keep string names in object literal#1108

Closed
yamafaktory wants to merge 6 commits intoprettier:masterfrom
yamafaktory:1103-keep-string-names-in-object-literal
Closed

Keep string names in object literal#1108
yamafaktory wants to merge 6 commits intoprettier:masterfrom
yamafaktory:1103-keep-string-names-in-object-literal

Conversation

@yamafaktory
Copy link
Copy Markdown
Contributor

This one should fix #1103.

The printProperty helper function was implemented as a workaround to
an existing Flow bug which no longer exists. Every function call
can be simply replaced by a path.call() invocation.
@ericclemmons
Copy link
Copy Markdown
Contributor

Heads up, there are merge conflicts on this PR! (I don't think Github notifies users if their PR gets blocked by this.)

@yamafaktory
Copy link
Copy Markdown
Contributor Author

@ericclemmons Sadly no notification for a merge conflict... but this should be fine now 😄.

@jlongster
Copy link
Copy Markdown
Member

I thought we did this on purpose (remove strings when they aren't necessary)?

@SimenB
Copy link
Copy Markdown
Contributor

SimenB commented Apr 13, 2017

If this is merged, I'd like an option for it. Not everyone (few, I'd wager) needs closure compiler optimizations

@yamafaktory
Copy link
Copy Markdown
Contributor Author

This initial issue mentioned both Google Closure Compiler & Uglify. IMHO, if you voluntarily stringify object literals' keys in your codebase, this should be kept.

@SimenB
Copy link
Copy Markdown
Contributor

SimenB commented Apr 13, 2017

IMHO, if you voluntarily stringify object literals' keys in your codebase, this should be kept.

Why should quoted keys be treated differently than other stuff like quotes, semis, linebreaks etc? Prettier's great strength (IMO) is enforcing consistency in all things by not (at least rarely) caring about input formatting at all. There should be strong arguments to change away from that, and while advanced optimizations is a strong argument, I do think most people (e.g. close to 100% of people writing for Node) would prefer the current behavior. So it's better to add an option.

@yamafaktory
Copy link
Copy Markdown
Contributor Author

As the initial issue highlighted it because this simple modification has consequences on how some tools are going to interpret it. You can see that difference in the AST, so why removing it? I don't see that the same way as switching e.g. from double to single quotes. But this is just my opinion.

@bakkot
Copy link
Copy Markdown
Collaborator

bakkot commented Apr 14, 2017

@yamafaktory I think it's a major deficiency of ESTree that it represents ({a: 0}) and ({'a': 0}) differently. They're not different, and it makes writing ESTree analysis and transformation more annoying; it's very much the sort of concrete syntax a good AST format would abstract.

@yamafaktory
Copy link
Copy Markdown
Contributor Author

yamafaktory commented Apr 25, 2017

@vjeux / @jlongster I've synced it with current master and updated the tests accordingly. Please decide on whether or not this is an improvement (thinking of Google Closure, etc) or a regression as it involves some debatable inconsistencies 👍.

@vjeux
Copy link
Copy Markdown
Contributor

vjeux commented May 5, 2017

So sorry I've been letting this one hang for a long time. I don't think we should do that. In practice, the number of people that managed to use the Google Closure Compiler with Advanced Mode is extremely tiny and I feel like it's better to normalize those keys.

@vjeux vjeux closed this May 5, 2017
@Quantumplation
Copy link
Copy Markdown
Contributor

The concerns raised in this make a lot of sense, but I thought I'd chime in with a similar but alternate use case.

const config: { [type: string]: FileMetadata } = {
  'har': {
    contentType: 'application/json',
    extension: 'har',
  },
  'page-source': {
    contentType: 'text/html',
    extension: 'html',
  },
  'browser-logs': {
    contentType: 'application/json',
    extension: 'json',
  }
};

In this case, we'd like the har key to be quoted to be consistent with the other entries in the map, which require the quoted keys. This corresponds to the "consistent-as-needed" setting here: https://palantir.github.io/tslint/rules/object-literal-key-quotes/

However, prettier will automatically strip out these single quotes, and I'd prefer not to litter prettier ignore flags all over our code base.

We're going to accept what prettier says for now, but I thought I'd raise this in case other people felt similarly.

@itsMapleLeaf
Copy link
Copy Markdown

I ran into this myself. It conflicts with one of my TSLint rules that requires consistent quoting in objects literals, and I think it's fair for prettier to go with this convention as well, to denote that if an object requires quotes for keys, that it's probably being used as a dictionary as opposed to an object with properties.

@kushal
Copy link
Copy Markdown

kushal commented Jun 29, 2017

@yamafaktory Did you think about trying to merge into https://github.com/arijs/prettier-miscellaneous ?

@yamafaktory
Copy link
Copy Markdown
Contributor Author

@kushal feel free to reuse the code from this PR if you plan or wish to have that feature in the fork you mentioned.

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 19, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow Object Literal to be Explicit with String Names

9 participants