Use Prettier for formatting core JavaScript

1. +++ b/core/.eslintrc.json
   @@ -35,7 +39,6 @@
   -    "brace-style": ["error", "stroustrup"],
   

   No c++ reference anymore, how sad.

2. +++ b/core/package.json
   @@ -28,15 +29,18 @@
   -    "eslint": "^3.19.0",
   -    "eslint-config-airbnb": "^14.1.0",
   -    "eslint-plugin-import": "^2.2.0",
   -    "eslint-plugin-jsx-a11y": "^4.0.0",
   -    "eslint-plugin-react": "^6.10.3",
   +    "eslint": "^4.9.0",
   +    "eslint-config-airbnb": "^16.1.0",
   +    "eslint-config-prettier": "^2.9.0",
   +    "eslint-plugin-import": "^2.7.0",
   +    "eslint-plugin-jsx-a11y": "^6.0.2",
   +    "eslint-plugin-prettier": "^2.6.0",
   +    "eslint-plugin-react": "^7.4.0",
        "glob": "^7.1.2",
   

   If I understood you correctly, we need to update eslint to get the prettier eslint config working. Seems reasonable given it is a dev configuration.

For the follow up, how about you creating the ticket, tag it as novice, and review it by yourself. I'm quite sure you get quicker endresults :)

Something is blocking us :( #2981652-4: [Aug. 9 2018] Format core JavaScript using recently add Prettier - running prettier shouldn't result in linting issues.

Here is updated patch.
What I did is just add the prettier without upgrading other packages (at least it's not required for getting prettier work).

p.s. I think upgrading eslint packages should be done in follow-up, because it introduces extra lint issues.

Here is follow-up for updating packages: #2982345: Update eslint packages to most recent major versions

Here is one big patch which includes patches from followups:
- #2982345: Update eslint packages to most recent major versions
- #2981652: [Aug. 9 2018] Format core JavaScript using recently add Prettier

but why not to take the patch from #6 which is just ~6KB that introduces prettier without any issues?

updating deps - we have followup.
reformatting by prettier - we have followup.

Up
but if all child issues are RTBC - then the big patch (#8) should be taken (it's just merge of all patches with no conflicts)

#14 looks clean and nice (flex)

I love Prettier and used it in each of my own projects. :p

I also considered it before push ESLint to Core (one step to the final). One of the problems I've seen, few of other huge projects outside Drupal also experienced it but they restored to ESLint at final (or dropped ESLint). Prettier is not as stable as it seems sometimes (at version update or whatever it can be wired, you will lose control and have to adapt their way), it will change pretty a lot of code. I'm not sure if Drupal would tolerate such an amount of new changes and to go back and fore.

A simple example, sometimes you change a LETTER but Prettier will rewrite a few lines.

Prettier isn't 100% always print IDENTICAL code. Reviewers can't distinguish Preitter or your own changes from a patch file.

Two questions:

1. When should contributors run lint:core-js vs. lint:core-js-passing vs. prettier? Should we just run "prettier" before rolling every patch? This isn't clear to me.
2. Does this need a change notice?

Adding tag.

This looks good to me - the change record mentions that the branch this is introduced in is "8.6.x", which relies on a backport from 8.7.x, but from talking with framework managers it sounds like there's a good chance of this getting into 8.6 before beta.

+++ b/core/package.json
@@ -17,7 +17,8 @@
+    "prettier": "prettier --write './**/*.es6.js' './tests/Drupal/Nightwatch/**/*.js'"

should be double quotes for Windows.

It's better to combine 2 commands (or as the new command). Prettier run first and then linting tools. lint-staged also a good tool and time-saver.

For #24.

Here is lint-staged solution which I really like.
But, let me know if we should go with it in this task, otherwise I will provide a patch to fix double quotes only.

Props:
- less chance to forget something to run
- prettier will process only staged files
- you can't commit if lint:core-js-passing fails
- and many more...

Cons (but not sure):
- you need to commit changes (via git commit) to run lint-staged commands automatically (e.g prettier)
- you can also add changes (via git add) and then run lint-staged manually via yarn run precommit

If this issue is completed in time to be backported to 8.6.x as per the alpha phase allowed changes, I would recommend backporting it and then scheduling #2981652: [Aug. 9 2018] Format core JavaScript using recently add Prettier for during the beta phase.

as I thought :) Here is updated patch.

Rerolled with 8.7.x

I've also added this rule "prettier/prettier": "off" to .eslintrc.passing.json
This will allow not to have lint issues from the prettier when yarn run lint:core-js-passing

it should be removed in #2981652: [Aug. 9 2018] Format core JavaScript using recently add Prettier

Oh, forgot to upload the patch :)

I applied #31

I then followed the instructions in the change record: https://www.drupal.org/node/2986680

I got a bunch errors when I ran yarn run lint:core-js-passing

Most of them where like this
62:20 error '?' should be placed at the end of the line operator-linebreak
This is because we added the line break rule in #2983365: JS codestyle: operator-linebreak.

But when run prettier it will make lines depending on it's own internal rules so we can't a rule on this and expect it to pass. So we should remove this rule.

The others where

/Users/ted.bowman/Sites/www/d8/core/modules/image/js/theme.es6.js
  70:22  error  Unexpected string concatenation of literals  no-useless-concat
  80:22  error  Unexpected string concatenation of literals  no-useless-concat

In this case it we are concatenating lines which prettier changes to all on the single so it becomes a useless concat.
This could be fixed in #2981652: [Aug. 9 2018] Format core JavaScript using recently add Prettier or on commit if the committer is going to run prettier.

/Users/ted.bowman/Sites/www/d8/core/modules/editor/js/editor.admin.es6.js
  294:13  error  'findPropertyValuesOnTag' was used before it was defined  no-use-before-define

The offending code is

// eslint-disable-next-line no-use-before-define
if (deleteFromUniverseIfAllowed(universe, tag)) {

We are using a comment here to disable linting a particular line.
When prettier is run the code will look something like

// eslint-disable-next-line no-use-before-define
if (
     deleteFromUniverseIfAllowed(universe, tag)) {

So here the disabling of eslint no longer works because it was pushed down 1 line.

Again this can be fixed in #2981652: [Aug. 9 2018] Format core JavaScript using recently add Prettier or on commit if the committer is going to run prettier.
We would simply need to move the comment down 1 line.
This is the problem @alexpott referred to #5

So I think the change is to remove the no-useless-concat rule which won't make sense prettier.

Thanks for review, but it seems you forgot to run yarn install in the ./core folder.

here is my result after applying patch and running yarn install && yarn run lint:core-js-passing:

https://www.dropbox.com/s/20hq3wal1iwzuch/Screenshot%202018-08-02%2013.2...

@ApacheEx thanks for all your work on this issue!

Thanks for review, but it seems you forgot to run yarn install in the ./core folder.

I did the yarn install

Looking at your screen shot you didn't actually run `yarn run prettier` from the change record instructions: https://www.drupal.org/node/2986680

The patch itself doesn't change any *.es6.js files so it will not have any effect on the output of yarn run lint:core-js-passing.

To commit this issue we have to make sure that the changes that `yarn run prettier` will make to the files are compatible with our eslint rules so that we don't always get errors with
yarn run lint:core-js-passing
This was @alexpott's point in #5

So I think the change is to remove the no-useless-concat rule which won't make sense prettier.

I copied in the wrong rule here. I meant operator-linebreak

The main point I was making in #32 is that operator-linebreak is not compatible with the new prettier changes. yarn run prettier will always produce code formatting that will not pass this rule.

Turning this rule off in .eslintrc.passing.json in reduces the errors after yarn run prettier is run from 99 to 3. #32 explains why we have these existing errors.

So we have to turn off operator-linebreak in this issue.

BTW I would do this change myself but I would rather not make any changes to this issue so I am free to RTBC this issue!(@ApacheEx and @drpal have already worked this issue a bunch so probably can't RTBC)

I confirm comments from @tedbow
I got 99 problems https://gist.github.com/dcorb/bdc8b04a649286d316b4e0723cecec21
but I think I got a solution

Instead of totally removing this line:
"operator-linebreak": ["error", "after"]
I changed it to
"operator-linebreak": ["error", "after", { "overrides": { "?": "ignore", ":": "ignore" } }]
(It seems is quite common to make an exception for ? and :, since the rule is provided in the documentation of eslint rule https://eslint.org/docs/rules/operator-linebreak )

Also I upgraded Prettier to 1.4.0 (the other 2 packages were latest)

There are still 3 eslint problems left, like Ted reported too: https://monosnap.com/file/F5PPdy7xgaWmrtc61DxTEVNdI5fX6w
But agree with @tedbow to apply necessary changes along with the issue that prettifies the files 🦄

It seems like prettier does some pretty interesting changes for our theme functions. Posting a patch containing all changes prettier does so we can confirm that there's no other instances where something would go wrong. In the instance that was failing due to eslint prettier did some changes that probably would have led into breaks even in the functionality. We should report that in the upstream repository.

I also applied a patch that fixes the eslint failures which can be applied after running prettier.

About failure test, see https://www.drupal.org/project/drupal/issues/2981652#comment-12664645 for more details

So, what you need to do is revert ./core/modules/locale/tests/locale_test.es6.js and add it to .prettierignore

Great ApacheEx. So, that means we are good to go with #35 patch and then we can move into #2981652: [Aug. 9 2018] Format core JavaScript using recently add Prettier, that is about "applying prettier". Patch from laurii in #36 can be moved there too. Nice fixes

@tedbow would you do the honours to give the final RTBC :)

@drpal & @ApacheEx thanks for all the work on this!

RTBC!🎉For the patch in #35

Committed #35 979164b and pushed to 8.7.x. Cherry picked 9a9a04b and pushed to 8.6.x. Thanks!

We include a brief list of coding standards changes in our release notes, since they affect core, contrib, and custom projects that use our standards.

The change record https://www.drupal.org/node/2986680 is a bit sparse. I don't understand what I'm supposed to be doing with it.

1. What does prettier do? The change record doesn't say. The prettier website isn't much help either. It's something to do with code formatting.
2. Following the instructions doesn't work...
   1. Are there any prerequisites, not mentioned in the change record? Needs Yarn and Nodejs, I already had those.
   2. Do the commands have to be run in a particular directory? Run the yarn commands in core/ directory. Run yarn install first.
3. What should I expect to happen?

Update: answered some of my own questions. We could do with a handbook page summarizing the various JS dev tools we now have, The details are scattered around various change records. I'm still sketchy on what prettier actually does though?

Is this lib referenced in core not installed when a d8 site is built with composer?

ESLint library loaded from: ...\web\themes\custom\motif\node_modules\eslint\lib\api.js
Cannot find module 'eslint-plugin-prettier' Referenced from: ...\web\core\.eslintrc.json Referenced from: ...\web\.eslintrc.json
ESLint library loaded from: C:\Users\James\AppData\Roaming\npm\node_modules\eslint\lib\api.js

#48,

Actually, I'm against this commit while we able to improve the whole experience with just a few mins or an hour max.

Basically, we should look into this #24. And the #2879072: Improve ES6 patching experience, it can output a friendly error message instead docs and it will be always up-to-date.

#49,
Run `yarn install` in CORE dir first.