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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide way to use single quotes in jsx #1080

Closed
trotzig opened this Issue Mar 23, 2017 · 52 comments

Comments

Projects
None yet
@trotzig
Contributor

trotzig commented Mar 23, 2017

I read this comment saying that "I've never seen anyone write <div className='a' />". Well, here's someone 馃憢

The same way trailingComma accepts a string, we could let singleQuote do as well.

  • 'all' would use single quotes in js as well as jsx
  • 'js' (or true) would use single quotes in js but not in jsx
  • 'jsx' would use single quotes in jsx but not in js
@Pajn

This comment has been minimized.

Contributor

Pajn commented Mar 23, 2017

Here's someone else!
We currently use tslint with auto fixes which also changes all " in jsx to ' so I guess people which use singlequotes and tslint is used to them in jsx too.

@trotzig

This comment was marked as off-topic.

Contributor

trotzig commented Mar 23, 2017

@Pajn Seems fitting that we're both from Sweden, the land of single households 馃嚫馃嚜

@iansinnott

This comment has been minimized.

iansinnott commented Mar 23, 2017

Same here! I actually thought it was some sort of config mistake on my end when I saw it replace single quotes with double quotes.

trotzig added a commit to trotzig/prettier that referenced this issue Mar 24, 2017

Modify `singleQuote` option to allow single quotes in jsx
We use jsx with single quotes in our application code:

  <Icon name='search' />

When I first tried out prettier, I thought that the `singleQuote` would
control jsx attributes as well. But I found out that it didn't.

It's likely that we are an outlier, so I didn't want to force
single-quotes all over. The solution I went with was to allow different
quote styles in regular js code and in jsx. The `singleQuote`
(--single-quote) option now takes a string that can have the following
values:

  "none" (default) - use double quotes everywhere
  "all" - use single quotes everywhere
  "js" - use single quotes in js code, double quotes in jsx
  "jsx" - use double quotes in jsx code, double quotes in js

Inspired by 5d13102, I kept support for the boolean option. It comes
with a deprecation warning, so people should find out that they have to
update soon.

Fixes prettier#1080
@jlongster

This comment has been minimized.

Member

jlongster commented Mar 27, 2017

We really, really don't want to add any more configuration options. Ideally when commas at the end of function parameter lists we can deprecate the options to trailing comma and just accept a single --trailing-comma option. The only reason we can't right now is because function arg trailing commas aren't valid JS, but people can use them with a babel transform.

I think the only option here is to decide if JSX should respect the quote option or not. This is the first I've heard someone complain about it which makes me think you're a small minority (sorry!). I think makes sense the way we currently do it since using double-quotes is a very strong style with HTML an that's why JSX applies the same practice.

It's worth having the discussion though. What do you think @rattrayalex and anyone else?

@trotzig

This comment has been minimized.

Contributor

trotzig commented Mar 27, 2017

I understand and support the desire to keep configuration options down. I was actually surprised to see singleQuote as an option when I first started using the tool, I thought the majority had switched over to single quotes already.

I could see a solution where we just keep whatever quotes are used in jsx. I don't know how well that rhymes with the philosophy of prettier however.

@rattrayalex

This comment has been minimized.

Collaborator

rattrayalex commented Mar 27, 2017

My impression is JSX style strongly encourages double-quotes; @trotzig I'm curious why you prefer single-quotes for JSX? Could double-quotes be acceptable?

EDIT: hello from react-waypoint, by the way! 馃憢

@trotzig

This comment has been minimized.

Contributor

trotzig commented Mar 27, 2017

Hi @rattrayalex!

Double quotes are definitely acceptable. It's a style thing we started doing a few years back, and I don't have strong feelings for or against it. I guess you could argue that single quotes makes it more obvious that we're not technically dealing with raw html (must use className instead of class etc). But yeah, we could fall into the mainstream here without much pain.

@jlongster

This comment has been minimized.

Member

jlongster commented Mar 27, 2017

I always feel bad saying no but I think it's good for the long-term health of the project. Thanks for being open to changing your style! It would be really easy for our config to get really complicated if we catered to a lot of the existing styles. I think it's going to be really valuable for the JS community if a large portion of the styles are decided on.

@jlongster jlongster closed this Mar 27, 2017

@joshma

This comment has been minimized.

joshma commented Mar 28, 2017

Hm I know you just closed this, but I wanted to make a push for consistency - for the same reason that --single-quote is a flag, someone who uses single quotes as a stylistic choice would want to use that for JSX as well. (That's certainly the case for us - we just see JSX as a nice wrapper around React.createElement.)

Can we consider having the JSX behavior match non-JSX behavior?

@rattrayalex

This comment has been minimized.

Collaborator

rattrayalex commented Mar 28, 2017

@joshma I think most requests for options need a pretty strong use case at this point. Pure style preference usually isn't enough; part of the goal of the project is to rally the community around a single "style preference" (which, of course, nobody will be perfectly happy with 鈥撀爄ncluding the project maintainers).

Would the status quo be usable for your team, or a blocker from using prettier?

@joshma

This comment has been minimized.

joshma commented Mar 28, 2017

@rattrayalex just to be clear, I'm not requesting another option, just that the JSX behavior matches the existing --single-quote option. (My claim is that most people are consistent between JSX and non-JSX code, although that could be wrong.)

It'd be a blocker somewhat, in the sense that I wouldn't switch our team over 馃槢 . I want to introduce prettier to reduce style discussions and encourage consistency, but in doing so we'd have to either convert our entire codebase (and in some sense create a lot of discussion) or accept inconsistency between "prettier" code and legacy code (which seems worse than what we have now).

FWIW, I totally accept that this project is allowed to make its own opinions on what's valuable for the JS community, but the value to my team is consistency in some style, not consistency in following a global style. And I think that's doable with the community at large also - you use whatever prettier config the codebase you're contributing to uses, and that's that.

Maybe we'll drift towards prettier's default config over time, but it seems better to gradually change things.

@rattrayalex

This comment has been minimized.

Collaborator

rattrayalex commented Mar 28, 2017

Hmm, interesting. Thanks for sharing @joshma !

most people are consistent between JSX and non-JSX code, although that could be wrong.

that's not what I've seen 鈥撀爓hich is a strong majority using the recommended " in JSX, despite many using ' elsewhere 鈥撀燽ut part of what makes this hard is that there isn't data anywhere, and everyone has different experiences 馃槙

@joshma

This comment has been minimized.

joshma commented Mar 28, 2017

Interesting, it that just so it matches common HTML style?

@rattrayalex

This comment has been minimized.

Collaborator

rattrayalex commented Mar 28, 2017

It may also be because you can't escape a quote within JSX:
http://eslint.org/docs/rules/jsx-quotes

but, yes, I think the typical answer is that: https://github.com/airbnb/javascript/tree/master/react#quotes

Regular HTML attributes also typically use double quotes instead of single, so JSX attributes mirror this convention.

This question, like most others of similar import, is not without its controversy: airbnb/javascript#269 (comment) and airbnb/javascript#629

@iansinnott

This comment has been minimized.

iansinnott commented Mar 28, 2017

Totally agree maintainers can do what they think is best.

I would just point out that @joshma is definitely not alone. This is also the reason my team uses single quote everywhere: consistency. In our case it's also simpler to tell new team members "single quote everywhere" rather than "single quote everywhere except JSX".

@lydell

This comment has been minimized.

Collaborator

lydell commented Mar 28, 2017

It's the simplest to tell new team members that they don't need to care about which quotes they type at all. prettier takes care of it ;)

@iansinnott

This comment has been minimized.

iansinnott commented Mar 28, 2017

Fair point @lydell

@mute mute referenced this issue Apr 14, 2017

Merged

Adds prettier command and runs it. #754

1 of 2 tasks complete
@audiolion

This comment has been minimized.

Contributor

audiolion commented Apr 15, 2017

I ran into this today, I thought it was a bug in prettier because I had set singleQuote: true and then arrived at this thread. I think it the comments for single quote in default settings should specific you only apply the option to .js and not .jsx if you are going to enforce this.

For me I can use prettier, and run eslint assets --ext .jsx --fix to convert all the double quotes prettier puts in back to single quotes, but I believe that either the docs or your idea is wrong in that

// If true, will use single instead of double quotes
"singleQuote": true,

should do what it says on the tin, and not have an undocumented exception for jsx

Perhaps

// If true, will use single instead of double quotes, except `.jsx` see https://github.com/prettier/prettier/issues/1080 for justification
"singleQuote": true,
@rattrayalex

This comment has been minimized.

Collaborator

rattrayalex commented Apr 16, 2017

@audiolion that makes sense! Care to submit a PR?

@audiolion

This comment has been minimized.

Contributor

audiolion commented Apr 16, 2017

Sure, wont be till Monday with Easter tomorrow, cheers!

@jamesgpearce

This comment has been minimized.

jamesgpearce commented May 4, 2017

A big +1 for single quotes in JSX - or rather, a consistent application of the flag. It seems strange to me that people would prefer one in JSX and the other in JS.

FWIW, in Babel, the transformed code appears to result in single quotes regardless:

import React from 'react';
const foo = (<Foo prop1="double" prop2='single' />);

becomes

import React from 'react';
const foo = React.createElement(Foo, { prop1: 'double', prop2: 'single' });

;)

@audiolion

This comment has been minimized.

Contributor

audiolion commented May 4, 2017

Oh man, I completely forgot about this! I will get this PR up today

@shinzui

This comment has been minimized.

shinzui commented May 31, 2017

@audiolion are you still planning a PR?

@audiolion

This comment has been minimized.

Contributor

audiolion commented Jun 1, 2017

Hah, yes, I am so sorry this keeps slipping my mind, it is on my Trello board now

@j-f1

This comment has been minimized.

Member

j-f1 commented Apr 18, 2018

Should this issue be unlocked to allow people to 馃憤?

@j-f1

This comment has been minimized.

Member

j-f1 commented May 4, 2018

Unlocking 馃攽 to allow people to 馃憤/馃憥.

Please don鈥檛 comment unless you have something to add that hasn鈥檛 already been said, and please don鈥檛 comment +1 or 馃憤 and instead use the 馃憤 reaction button on the original comment to indicate your support.

@baelec

This comment has been minimized.

baelec commented May 11, 2018

If this proposal is accepted, I will gladly submit the patch. I already submitted a PR in the past and am more than willing to make whatever changes are necessary.

@karlwilbur

This comment has been minimized.

karlwilbur commented May 19, 2018

The Rational clearly says:

JSX always uses double quotes. JSX takes its roots from HTML, where the dominant use of quotes for attributes is double quotes. Browser developer tools also follow this convention by always displaying HTML with double quotes, even if the source code uses single quotes.

However, the (X)HTML/SGML standards, as long as I have been writing HTML (over 20 years) have been to allow both single and double quotes for attributes.

HTML5

HTML 4

HTML 3.2

The "convention" has always been to allow both. That someone feels that one is preferred over the other is completely arbitrary. There are reasons to use one over the other and both are valid.

The statement that "dominant use of quotes for attributes is double quotes" seems to be entirely untrue. There is no evidence at all to support that claim. From my own personal experience, I always use single quotes for strings unless there is a reason to use double quotes, this includes HTML attributes. I know that most of my peers also use the same convention.

Clearly actual HTML standards should drive specification, not someone's feeling that something is "dominant" or "typical", and certainly when there is no evidence to support such a claim of dominance.

I am adding this comment because no one has mention that supporting both is the actual HTML standard.

Also, since someone asked this of another earlier, I would like to say that this seems to be a blocker for my team. We cannot use prettier because our convention is to use single quotes unless we are doing interpolation or when having to escape single quotes makes things not easily readable (JavaScript Standard Style). We use standard on save, and prettier and eslint on pre-commit. I will look into changing the flow from eslint -> prettier to prettier -> eslint on pre-commit, perhaps using prettier-eslint. Thanks to @lydell, @audiolion, and @shinzui for your comments, I hope they prove helpful.

@ljharb

This comment has been minimized.

ljharb commented May 19, 2018

A dominant convention is a defacto standard.

@smirea

This comment has been minimized.

Contributor

smirea commented Jun 14, 2018

bump.
i know there's a comment above saying not to +1, but the original comment has 62 馃憤 already and this issue has been stale for a month now

@j-f1

This comment has been minimized.

Member

j-f1 commented Jun 28, 2018

@smirea Feel free to open a PR! (Perhaps my suggestion would make a good starting point?)

@smirea

This comment has been minimized.

Contributor

smirea commented Jun 28, 2018

sure thing! I thought there already was a PR for this but I guess I miss-remembered

@j-f1 this is a working proof of concept based on your suggestion, seems pretty straightforward but want to make sure I'm not overlooking something

cat <<CODE > /tmp/test-prettier.js && echo '------' && ./bin/prettier.js /tmp/test-prettier.js --single-quote=all
const a = "foo";
const jsx = <div style="color:red" this-should='stay the same'>some quotes "that should not" change</div>
CODE

if it looks good I can go ahead and write some tests and open a PR

@j-f1

This comment has been minimized.

Member

j-f1 commented Jun 28, 2018

That looks great!

@baelec

This comment has been minimized.

baelec commented Jun 30, 2018

@smirea There was! I submitted it a while back but eventually it withered on the vine. It'd need to be updated with the latest suggestions from @j-f1 and rebased as well. If it helps, you can find the work I did here. I don't mind updating it if that's the route we want to go but it sounds like you're already doing quite well without it. Thanks for looking into this!

Edit: One final thing that I remembered... Not sure if @j-f1 still wants it or not, but he had suggested some changes regarding replacing &apos; which you can find in my PR in printer-estree.js:1640.

@smirea

This comment has been minimized.

Contributor

smirea commented Jul 3, 2018

Thanks all, I submitted a preliminary PR if y'all can take a gander and give some feedback

smirea added a commit to smirea/prettier that referenced this issue Jul 3, 2018

@bovas85

This comment has been minimized.

bovas85 commented Aug 13, 2018

Anyone here can review/approve that PR please?

@j-f1 j-f1 added status:has pr and removed help wanted labels Aug 23, 2018

@konpikwastaken

This comment has been minimized.

konpikwastaken commented Sep 27, 2018

Gentle ping to maintainers on this PR - can anyone take a look?

@trotzig

This comment has been minimized.

Contributor

trotzig commented Sep 28, 2018

@konpikwastaken I opened this PR over a year ago and have since moved on to using double quotes. I have no desire to pick it up.

@bovas85

This comment has been minimized.

bovas85 commented Sep 28, 2018

well we can't really approve this, but someone should.

If you are not drowning, it doesn't mean everyone else isn't.

@trotzig

This comment has been minimized.

Contributor

trotzig commented Sep 28, 2018

I鈥檓 not a maintainer of this project, so even if I wanted to I couldn鈥檛 merge this. I鈥檓 going to close this, and I encourage discussion/work to continue elsewhere.

EDIT: apologies, I thought I was commenting on my pull request...

@trotzig trotzig closed this Sep 28, 2018

@trotzig trotzig reopened this Sep 28, 2018

@jstnjns jstnjns referenced this issue Oct 2, 2018

Merged

Form elements #221

@ikatyang ikatyang modified the milestones: 2.0, 1.15 Nov 4, 2018

@j-f1 j-f1 closed this in #4798 Nov 4, 2018

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