Problem/Motivation

Currently, we are using a mixture of single quotes and double quotes in our JavaScript, and I can't see why. When I turn on eslint's quotes rule set to single quotes, it only returns 215 errors; pretty small compared to turning on the eslint rule and setting it to double quotes: 4013 errors.

Our PHP standards for quotes are pretty clear: https://www.drupal.org/coding-standards#quotes

single quote strings should be used by default. Their use is recommended except in two cases:

  1. Deliberate in-line variable interpolation, e.g. "<h2>$header</h2>".
  2. Translated strings where one can avoid escaping single quotes by enclosing the string in double quotes. One such string would be "He's a good person." It would be 'He\'s a good person.' with single quotes. Such escaping may not be handled properly by .pot file generators for text translation, and it's also somewhat awkward to read.

In JavaScript, there is no such thing as #1. But we do have Drupal.t() which can have strings with apostrophes.

So I think we should adopt the same rules for quotes in JavaScript as we already have for PHP.

Fortunately, eslint has a rule that will allow us to: always use single quotes, except when we use double quotes to avoid escaping a single quote or apostrophe in the string.

Proposed resolution

Let's update our .eslintrc file to add:

"quotes": [2, "single", "avoid-escape"]

Remaining tasks

  1. Add the new eslint rule.
  2. Fix the 211 linting errors by switching string from double quotes to single quotes.
  3. Update Drupal's JavaScript docs to mention the new recommendation. https://www.drupal.org/node/172169

    Quotes

    Single quote strings should be used by default. Their use is recommended except in the following case:

    Translated strings where one can avoid escaping single quotes by enclosing the string in double quotes. One such string would be Drupal.t("She's a good person.") It would be Drupal.t('She\'s a good person.') with single quotes. Such escaping may not be handled properly by .pot file generators for text translation, and it's also somewhat awkward to read.

CommentFileSizeAuthor
#22 core-js-eslint-quotes-2548195-22.patch68.61 KBnod_
#21 only_use_single_quotes-2548195-21.patch68.17 KBsriharsha.uppuluri
#12 core-js-eslint-quotes-2548195-12.patch68.6 KBmadhavvyas
#11 core-js-eslint-quotes-2548195-10.patch68.6 KBmadhavvyas
#8 core-js-eslint-quotes-2548195-8.patch68.22 KBnod_
#8 interdiff.txt1.16 KBnod_
#7 eslint-quotes-2548195-7.patch75.74 KBjohnalbin
#2 eslint-quotes-2548195-2.patch75.84 KBjohnalbin

Comments

JohnAlbin created an issue. See original summary.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new eslint-quotes-2548195-2.patch75.84 KB

Here's a patch.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Didn't have the courage to fill this issue but it's very much +1 from me.

Only thing is the change in active links. I could swear I needed the single quotes because querySelector didn't like some values of queryString but I can't remember I dislike double quotes so if I used them like this there is probably a reason.

The rest is fine.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

Only thing is the change in active links. I could swear I needed the single quotes because querySelector didn't like some values of queryString but I can't remember I dislike double quotes so if I used them like this there is probably a reason. 

diff --git a/core/misc/active-link.js b/core/misc/active-link.js
@@ -26,7 +26,7 @@
       // Start by finding all potentially active links.
       var path = drupalSettings.path;
       var queryString = JSON.stringify(path.currentQuery);
-      var querySelector = path.currentQuery ? "[data-drupal-link-query='" + queryString + "']" : ':not([data-drupal-link-query])';
+      var querySelector = path.currentQuery ? '[data-drupal-link-query="' + queryString + '"]' : ':not([data-drupal-link-query])';
       var originalSelectors = ['[data-drupal-link-system-path="' + path.currentPath + '"]'];
       var selectors;

Do you want me to reverse that hunk? And add an eslint exclude comment?

I couldn't find anything in the docs for querySelectorAll() that hints that quotes make any difference. Neither https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelectorAll nor http://www.w3.org/TR/selectors-api/

nod_’s picture

nod_
Primary language French
Location Lille
commented

maybe it was with <front>. Not sure, I'll try to remember, if I take too long let's go with it.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work

Ah! remembered. It's because queryString is a JSON value with " all over the place. Faster to do it that way than escape everything. So we have to exclude that line from eslint check.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Status: Needs work » Needs review
StatusFileSize
new eslint-quotes-2548195-7.patch75.74 KB

new patch!

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new interdiff.txt1.16 KB
new core-js-eslint-quotes-2548195-8.patch68.22 KB

Rerolled since the sec critical added a bunch of lines in ajax.js other than that, all good!

nod_’s picture

nod_
Primary language French
Location Lille
commented

When testing, know that eslint released a new version 1.3.0, it's bugged so use eslint 1.2.0 or wait for https://github.com/eslint/eslint/issues/3570 to be fixed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs beta evaluation, +Needs reroll

This no longer applies after JSDoc-ageddeon.

This could also use a beta eval, as it seems like quite a disruptive change for little gain?

madhavvyas’s picture

madhavvyas commented
Issue tags: -Needs reroll
StatusFileSize
new core-js-eslint-quotes-2548195-10.patch68.6 KB

Patch re-rolled comment #8

madhavvyas’s picture

madhavvyas commented
StatusFileSize
new core-js-eslint-quotes-2548195-12.patch68.6 KB

Updated patch file name, Not sure why Add test link not coming beside re-rolled patch.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

I hadn't realized madhavvyas had created an updated patch, so I independently re-rolled my patch to the latest 8.0.x. When running with eslint 1.3.1, I was seeing new JS code fail the linters, so I fixed the new double-quoted JS and then created a new patch.

Then I noticed madhavvyas' patch. And our patches are identical! So his patch in #12 looks good to go.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

Agreed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Postponed
Issue tags: +revisit before release

This is exactly the sort of coding standards change that will be good to make during RC rather than just before it. Since it is disruptive to patches but undisruptive to codebase. Let's do it then.

droplet’s picture

droplet commented

In case you don't know, it can perform autofix with this command:

eslint --fix core\**\*.js

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -revisit before release +rc target

The tag for #16 is "rc target".

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Postponed » Needs work

Needs a reroll

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Issue tags: +Needs reroll

Adding tag

sriharsha.uppuluri’s picture

sriharsha.uppuluri commented
Status: Needs work » Needs review
StatusFileSize
new only_use_single_quotes-2548195-21.patch68.17 KB

Its re-rolled.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new core-js-eslint-quotes-2548195-22.patch68.61 KB

Missing one 

core/modules/responsive_image/js/responsive_image.ajax.js
  3:3  error  Strings must use singlequote  quotes

✖ 1 problem (1 error, 0 warnings)

Updated patch to fix this issue (another case of "use strict";).

Tried it, no js error or anything, everything looks good.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs beta evaluation, -Needs reroll

As agreed this can be committed during RC.

Committed b0e82e5 and pushed to 8.0.x. Thanks!

  • alexpott committed b0e82e5 on 8.0.x 
    Issue #2548195 by nod_, JohnAlbin, madhavvyas, sriharsha.uppuluri: Only...
johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

Thanks, Alex!

And Sri, Théodore, and madhavvyas, too!

:-)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.