New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to print single quotes in JSX #4798

Merged
merged 8 commits into from Nov 4, 2018

Conversation

Projects
None yet
10 participants
@smirea
Contributor

smirea commented Jul 3, 2018

Extending the single-quote option to allow using single quotes in JSX.
single-quote has been changed from a bool to a choice: none (false), js (true), all (new functionality, includes JSX)

Functionality based on this comment from the long issue thread

Fixes #1080.

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

@smirea smirea changed the title from Feat option single quote jsx to Feat option single quote jsx, fixes #1080 Jul 3, 2018

const hasDouble = innerValue.includes('"');
if (options.singleQuote === "all") {
if (hasSingle && hasDouble) {

This comment has been minimized.

@lydell

lydell Jul 3, 2018

Collaborator

I think we should count the number of singles and doubles and choose surrounding quotes so that the number of escapes is minimized, like we do for JS strings.

But wait a minute – can there ever be both 's and "s at the same time in an attribute value? I don't think so. And is there are risk of getting all 's and "s turned into ' and " as you run Prettier in between edits? Should we unescape unnecessary ' and "?

This comment has been minimized.

@smirea

smirea Jul 3, 2018

Contributor

valid point, I just realized as well that it's not valid to escape quotes in pure JSX attributes, i.e.

this is invalid: <div id="invalid \" jsx" />
this works though: <div id={"valid \" jsx"} />

I'm updating the format to use the same logic from printString, there's a previous thread going through this discussion in more depth above

This comment has been minimized.

@Kovensky

Kovensky Jul 4, 2018

It's not invalid; it just won't do what you mean 😄

It's better to just convert to string expressions if you need any escaping.

@suchipi

I really don't think we should add this option... arrow parens I was sympathetic to because it's annoying to eg. convert an identifier to destructuring syntax, and I'm open to space between function name and params list because it makes the code easier to grep. But single/double quotes is just nitpicking, and I don't want the teams that use prettier to have to have a conversation about this.

I know this is unintuitive, since prettier is a code formatter, but in my opinion, the major value of prettier is that it makes you stop caring about syntax. It is not what people expect at first but once you let go it is very freeing to "let the formatting be what it will" and focus on the problems the code is solving instead of the aesthetic of the code.

@rattrayalex

This comment has been minimized.

Collaborator

rattrayalex commented Jul 3, 2018

@suchipi thoughts on putting this to a private vote amongst the maintainers or something? Perhaps over email?

We're all in agreement that we hate bikeshedding discussions, but it's not clear that there's overwhelming evidence one way or another on this one (eg; community demand seems strong, including from some luminaries). Eg; prettier's existence/stance doesn't seem to prevent teams from having conversations about this, it's just "do we use prettier when it lacks this feature".

@Kovensky

This comment has been minimized.

Kovensky commented Jul 4, 2018

I can say that I haven't yet adopted prettier specifically because of its lack of support for single quote JSX attributes. We don't write conversational english in our raw attributes, there is no need to care about the need to handle a potential apostrophe in your text, and forcing the attributes to be double quotes when all your other strings are single quotes is disorienting.

But I'm not an influencer I guess.

All this discussion was already had in the long issue thread that OP mentioned.

@rattrayalex

This comment has been minimized.

Collaborator

rattrayalex commented Jul 6, 2018

Yeah @Kovensky let's please avoid rehashing conversations 😄, it's clear from #1080 there's strong community demand, and countless reasons have been mentioned in that thread.

It's a clear tradeoff between "give the people what they want" and "never add options", which is just up to the maintainers.

In any case @smirea thanks for the work on this PR – the better it looks, the more likely it'll be accepted, I hope 🙂

@smirea

This comment has been minimized.

Contributor

smirea commented Jul 23, 2018

hi all, bump on this. I think I've addressed all the pending questions in the last commit from a few weeks ago.

Can y'all take another look so I can do a final rebase

@suchipi

This comment has been minimized.

Member

suchipi commented Jul 23, 2018

I still don't think that we should add this, but if we do, for what it's worth, some plugins are relying on singleQuote being boolean; I'm not sure if they can use support info to figure out how they're supposed to handle that.

@ikatyang

This comment has been minimized.

Member

ikatyang commented Jul 24, 2018

We should change options to be language-specific, e.g. "javascript/singleQuote": "something", though not sure how should it work.

@smirea

This comment has been minimized.

Contributor

smirea commented Jul 24, 2018

this feature is backwards compatible with the old signature via the config.
right now singleQuote: true redirects to singleQuote: js and false converts to none with an accompanying deprecation warning, so old modules will still work.

We could support all 5 options (true/js false/none all) without a deprecation to not have it be slightly awkward for non-js projects, my 2 cents is that it's better to bite the bullet sooner rather than later

Example of the current behavior:

screen shot 2018-07-24 at 9 48 02 am

@suchipi

This comment has been minimized.

Member

suchipi commented Jul 24, 2018

@smirea I mean like here: https://github.com/prettier/plugin-lua/blob/7e41cece6200de35244e1bea118452cb5efad5fa/src/printer.js#L570

Any prettier plugin could be relying on that option being a boolean.

@bovas85

This comment has been minimized.

bovas85 commented Aug 12, 2018

Any luck on this to be merged if approved?

@rattrayalex

This comment has been minimized.

Collaborator

rattrayalex commented Aug 17, 2018

Quick 👍 / 👎 – maintainers only please – @j-f1 @suchipi @lydell @ikatyang @vjeux @jlongster @azz (and any other maintainers who are watching) – should we merge this?

@ikatyang

This comment has been minimized.

Member

ikatyang commented Aug 17, 2018

I have no opinion on this PR but it shouldn't break other plugins #4798 (comment).

@rattrayalex

This comment has been minimized.

Collaborator

rattrayalex commented Aug 17, 2018

@ikatyang - agreed that that's a blocker to merge, though I'm guessing the author of this PR would rather not deal with implementing a solution for that until it's clear we want the change.

@smirea

This comment has been minimized.

Contributor

smirea commented Aug 17, 2018

I'm happy to fix the blockers if there is a desire to merge this, there's still a significant community desire for this feature :)

@bovas85

This comment was marked as outdated.

bovas85 commented Aug 18, 2018

I think this is a very much needed feature as it blocks everyone using standard.js along with prettier

@j-f1 j-f1 changed the title from Feat option single quote jsx, fixes #1080 to Add an option to print single quotes in JSX Aug 18, 2018

@ikatyang ikatyang referenced this pull request Aug 28, 2018

Merged

refactor: extract options-normalizer/validator #5020

1 of 1 task complete

ikatyang added a commit that referenced this pull request Aug 31, 2018

refactor: extract options-normalizer/validator (#5020)
- Uses [`vnopts`](https://github.com/ikatyang/vnopts#readme)
- This way it should be easier to support language-specific options (#4798 (comment)) and map the common options to language-specific options using [`forward`](https://github.com/ikatyang/vnopts#forward), e.g. `singleQuote: true` -> `"javascript/singleQuote": "js"`, `singleQuote: false` -> `"javascript/singleQuote": "none"`.
@j-f1

This comment has been minimized.

Member

j-f1 commented Sep 27, 2018

Quick vote for @prettier/core (only maintainers please): 👍 if this should be merged, 👎 if it shouldn’t.

@lydell

This comment has been minimized.

Collaborator

lydell commented Sep 27, 2018

What about always, never, except-jsx as the option values?

@j-f1

This comment has been minimized.

Member

j-f1 commented Sep 27, 2018

How about false, true, and @lydell’s "except-jsx"? This would mean that falsy values mean double quotes and truthy values single quotes, and the JS parser would look more closely at the exact values.

@ikatyang

This comment has been minimized.

Member

ikatyang commented Sep 27, 2018

FYI, we could forward singleQuote to jsSingleQuote to avoid breaking change, see #5020 for more info.

@smirea

This comment has been minimized.

Contributor

smirea commented Oct 27, 2018

reviving this with updates, added jsSingleQuote and forwarding it to singleQuote as per @ikatyang 's suggestion so now this is fully backwards compatible. 🎉

Can I get some final feedback on this and what would be the ideal format before I finalize tests and update snapshots?

@ikatyang

This comment has been minimized.

Member

ikatyang commented Oct 28, 2018

After second thought, it actually can be an independent bool option (--jsx-single-quote), there's no need to forward them (unnecessary complexity). What do you think?

@smirea

This comment has been minimized.

Contributor

smirea commented Oct 28, 2018

Personally personally, I would've not even added an extra option and have jsx follow js quotes and not have the notion of "we use a type of quote everywhere in js, except for this place" ... but I get how that is not possible now

In lieu of that, I think the --jsx-single-quote actually makes the most sense, it did feel a bit weird having an option override another

I'll give it a day and if there aren't any other suggestions i'll go ahead and add --jsx-single-quote and wrap it up

@j-f1

This comment was marked as resolved.

Member

j-f1 commented Oct 28, 2018

An idea I had recently: (this may be completely crazy)

  • singleQuote: false: never output single quotes (except in e.g. '"')
  • singleQuote: true: always output single quotes (except in e.g. "'")
  • singleQuote: ['js']: single quotes only in JS
  • singleQuote: ['jsx', 'foo', 'bar']: single quotes in JSX but not JS, plugins can make use of the foo and bar values.

This preserves backward compatibility since singleQuote: true matches everything. It also allows simple plugins to check the truthiness of the singleQuote option and since we check singleQuote.includes('jsx'), other plugins can add their own customizations if needed.


Is this excessively complex?

@lydell

This comment was marked as resolved.

Collaborator

lydell commented Oct 28, 2018

Is this excessively complex?

Yes.

@smirea

This comment has been minimized.

Contributor

smirea commented Oct 29, 2018

Ok I've updated the PR with:

  • use jsx-single-quote plain ol' boolean, removed all the vnopts magic
  • updated docs
  • updated snapshots

Can y'all have another gander and see if this checks out?

Show resolved Hide resolved docs/options.md Outdated
Show resolved Hide resolved docs/options.md Outdated
Show resolved Hide resolved docs/options.md Outdated
Show resolved Hide resolved tests/markdown/__snapshots__/jsfmt.spec.js.snap Outdated
Show resolved Hide resolved tests/quotes/jsfmt.spec.js Outdated
@smirea

This comment has been minimized.

Contributor

smirea commented Nov 1, 2018

hey all, all comments were addressed and everything is done from my end in the last commit.
Any final thoughts? If not, could we look into actually merging this finally? 😄

Show resolved Hide resolved src/common/util.js Outdated
@lydell

lydell approved these changes Nov 2, 2018

@suchipi

suchipi approved these changes Nov 3, 2018

@ikatyang

This comment has been minimized.

Member

ikatyang commented Nov 4, 2018

@j-f1 since you requested changes before, would you like to do a final review?

@ikatyang ikatyang added this to the 1.15 milestone Nov 4, 2018

ikatyang added a commit to ikatyang/prettier that referenced this pull request Nov 4, 2018

@j-f1

j-f1 approved these changes Nov 4, 2018

:shipit: 🚢 🚀

@j-f1

This comment has been minimized.

Member

j-f1 commented Nov 4, 2018

Just needs conflicts resolved.

@smirea

This comment has been minimized.

Contributor

smirea commented Nov 4, 2018

conflicts fixed, snapshots updated, checks passed, we're green across the board!

@j-f1 j-f1 merged commit e17512a into prettier:master Nov 4, 2018

10 checks passed

ci/circleci: build_prod Your tests passed on CircleCI!
Details
ci/circleci: checkout_code Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node4 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node9 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_standalone Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 80%)
Details
codecov/project 97% (+<.01%) compared to f6a95dd
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@j-f1

This comment has been minimized.

Member

j-f1 commented Nov 4, 2018

Thank you for contributing to Prettier!

@smirea smirea deleted the smirea:feat-option-singleQuote-jsx branch Nov 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment