Skip to content

Conversation

mattgrill
Copy link
Contributor

  • prettier
  • run prettier

"printWidth": 80,
"semi": true,
"singleQuote": true,
"trailingComma": "all"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love trailing comma all!

.eslintrc.json Outdated
{
"extends": [
"eslint-config-airbnb",
"plugin:prettier/recommended"
Copy link
Contributor

@dawehner dawehner Mar 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to compare what eslint-plugin-prettier does: https://github.com/prettier/eslint-plugin-prettier#recommended-configuration vs. what https://github.com/facebook/create-react-app/blob/next/packages/eslint-config-react-app/index.js does. Given we already try to go down the airbnb ruleset in core, I think using the same here makes sense.

package.json Outdated
"react-scripts": "1.1.1"
},
"scripts": {
"prettier": "./node_modules/.bin/prettier --write 'src/**/*.jsx' 'src/**/*.js'",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you just use "prettier"? It'll pick up things in node_modules/.bin/prettier automatically

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you can. I just always try and be specific.

Copy link
Contributor

@dawehner dawehner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from me. Following airbnb instead of the custom ruleset of CRA seems sensible (it'll also avoid conversations when we merge it into core)

Copy link
Contributor

@justafish justafish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When CRA is running the dev server it's running eslint automatically on re-compile - is it running this new configuration instead of default?

.eslintrc.json Outdated
"rules": {
"no-param-reassign": [0],
"no-underscore-dangle": [0],
"react/jsx-filename-extension": [1, { "extensions": [".js", ".jsx"] }]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ditch jsx completely

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can, however there are some tests that are .js but they contain JSX. I don't care either way which is why I added this.

@mattgrill
Copy link
Contributor Author

mattgrill commented Mar 8, 2018

@justafish I just looked into this, if you add an .eslintrc.json it picks it up.

.eslintrc.json Outdated
"root": true,
"env": {
"browser": true,
"es6": true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • jest, and serviceworker

@justafish
Copy link
Contributor

Just the linting errors themselves that need fixing now 👍

@justafish
Copy link
Contributor

So one thing I don't understand about adding eslint-config-prettier is it says that it "Turns off all rules that are unnecessary or might conflict with Prettier." - so what is enforcing the rules that it's turned off? Running prettier is something that each person needs to have done manually (or setup to do automatically on save, but still). If that is the case, then I would have thought our prettier config would be setup to help you follow the standards we've adopted (airbnb in this case), and so I'm not sure what this plugin actually does?

@justafish
Copy link
Contributor

Ok I read https://prettier.io/docs/en/eslint.html and I get it now 👍

@justafish justafish added the Help Wanted ⛑ Extra attention is needed label Mar 9, 2018
@mattgrill mattgrill removed the Help Wanted ⛑ Extra attention is needed label Mar 9, 2018
@justafish justafish merged commit 3e9343e into master Mar 9, 2018
@justafish justafish deleted the prettier branch March 9, 2018 19:16
VD39 pushed a commit to VD39/drupal-admin-ui that referenced this pull request Jul 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants