Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Updating ESlint to ^4.15.0 and adding new rules to config #3723

Merged
merged 11 commits into from
Jan 13, 2018
Merged

Updating ESlint to ^4.15.0 and adding new rules to config #3723

merged 11 commits into from
Jan 13, 2018

Conversation

chrislaughlin
Copy link
Contributor

@chrislaughlin chrislaughlin commented Jan 9, 2018 •
edited
Loading

Fixes #3341

I have updated the eslint dependency to 4.15.0 and enabled some non-controversial rules.

The newest rules added since the previous version 4.4.x are below, rules starred have been enabled:

This has not affected any code and no fixes have been needed. Let me know if anything else is needed or more/less rules should be enabled.
Thanks

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@gaearon
Copy link
Contributor

gaearon commented Jan 9, 2018

Seems like our code doesn't pass our preset now.

gaearon
gaearon previously requested changes Jan 9, 2018
View reviewed changes
.eslintrc Outdated
"curly": "warn"
"curly": "warn",
"getter-return": "error",
"function-paren-newline": ["error", "multiline"],
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can see this is a style rule. We don't enable any style rules in this preset.

@chrislaughlin
Copy link
Contributor Author

I have removed the style based rule and ran --fix over the packages. Turns out I was not running the correct eslint command. Is there a standard for running this across all the packages?

Fingers crossed this passes 🤞

gaearon
gaearon reviewed Jan 9, 2018
View reviewed changes
.eslintrc Outdated
"curly": "warn"
"curly": "warn",
"getter-return": "error",
"implicit-arrow-linebreak": ["error", "beside"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like another style rule?

@chrislaughlin
Copy link
Contributor Author

Removed implicit-arrow-linebreak rule. and updated PR description to reflect added rules.

gaearon
gaearon reviewed Jan 9, 2018
View reviewed changes
.eslintrc Outdated
@@ -12,6 +12,7 @@
"rules": {
"no-console": "off",
"strict": ["error", "global"],
"curly": "warn"
"curly": "warn",
"getter-return": "error"
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this our own eslintrc? To update the rules of our config, you'd need to change eslint-config-react-app/index.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good catch, I'll update the config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it worth keeping this new rule in the root eslint config for development work?

@chrislaughlin
Copy link
Contributor Author

I have added the new rule to eslint-config-react-app/index.js. It is still in the project eslint config but can remove if not needed.

gaearon
gaearon reviewed Jan 9, 2018
View reviewed changes
packages/eslint-config-react-app/index.js Outdated
@@ -239,6 +239,7 @@ module.exports = {
'Please use import() instead. More info: https://github.com/facebookincubator/create-react-app/blob/master/packages/react-scripts/template/README.md#code-splitting',
},
],
'getter-return': 'error',
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 make this a warning rather than an error?

gaearon
gaearon reviewed Jan 9, 2018
View reviewed changes
package.json Outdated
@@ -14,7 +14,7 @@
"precommit": "lint-staged"
},
"devDependencies": {
"eslint": "^4.4.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

This version is not very relevant to our users. You want to change the one in packages/react-scripts/package.json instead.

gaearon
gaearon reviewed Jan 9, 2018
View reviewed changes
packages/react-scripts/package.json Outdated
@@ -32,7 +32,7 @@
"chalk": "1.1.3",
"css-loader": "0.28.7",
"dotenv": "4.0.0",
"eslint": "4.10.0",
"eslint": "^4.15.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be pinned, like before.

@chrislaughlin
Copy link
Contributor Author

I have changed the rule to warn and pinned the version of eslint. :)

@gaearon gaearon changed the base branch from master to next January 12, 2018 20:07
@chrislaughlin
Copy link
Contributor Author

@gaearon thanks for all your help with this PR. 😃

@Timer Timer added this to the 2.0.0 milestone Jan 12, 2018
@Timer Timer merged commit 3861eeb into facebook:next Jan 13, 2018
@Timer
Copy link
Contributor

Timer commented Jan 13, 2018

Thanks!

gaearon pushed a commit that referenced this pull request Jan 13, 2018
@chrislaughlin @gaearon
Enable getter-return rule as a warning (#3723)
7e27126 
* Updating ESlint to ^4.15.0 and adding new rules to config

* remoning style rule and auto fixing breakages from new rules

* Removing implicit-arrow-linebreak style rule

* adding new rule to eslint config project

* updating react scripts eslint version

* Pinning version.

* Changing getter-return to warn

* Update package.json

* Update .eslintrc
Timer pushed a commit that referenced this pull request Jan 14, 2018
@chrislaughlin @Timer
Enable getter-return rule as a warning (#3723)
db8664c 
* Updating ESlint to ^4.15.0 and adding new rules to config

* remoning style rule and auto fixing breakages from new rules

* Removing implicit-arrow-linebreak style rule

* adding new rule to eslint config project

* updating react scripts eslint version

* Pinning version.

* Changing getter-return to warn

* Update package.json

* Update .eslintrc
gaearon pushed a commit that referenced this pull request Jan 14, 2018
@chrislaughlin @gaearon
Enable getter-return rule as a warning (#3723)
2973793 
* Updating ESlint to ^4.15.0 and adding new rules to config

* remoning style rule and auto fixing breakages from new rules

* Removing implicit-arrow-linebreak style rule

* adding new rule to eslint config project

* updating react scripts eslint version

* Pinning version.

* Changing getter-return to warn

* Update package.json

* Update .eslintrc
gaearon pushed a commit that referenced this pull request Jan 14, 2018
@chrislaughlin @gaearon
Enable getter-return rule as a warning (#3723)
2573f1a 
* Updating ESlint to ^4.15.0 and adding new rules to config

* remoning style rule and auto fixing breakages from new rules

* Removing implicit-arrow-linebreak style rule

* adding new rule to eslint config project

* updating react scripts eslint version

* Pinning version.

* Changing getter-return to warn

* Update package.json

* Update .eslintrc
gaearon pushed a commit that referenced this pull request Jan 14, 2018
@chrislaughlin @gaearon
Enable getter-return rule as a warning (#3723)
527fdfe 
* Updating ESlint to ^4.15.0 and adding new rules to config

* remoning style rule and auto fixing breakages from new rules

* Removing implicit-arrow-linebreak style rule

* adding new rule to eslint config project

* updating react scripts eslint version

* Pinning version.

* Changing getter-return to warn

* Update package.json

* Update .eslintrc
gaearon pushed a commit that referenced this pull request Jan 14, 2018
@chrislaughlin @gaearon
Enable getter-return rule as a warning (#3723)
7a4f5bc 
* Updating ESlint to ^4.15.0 and adding new rules to config

* remoning style rule and auto fixing breakages from new rules

* Removing implicit-arrow-linebreak style rule

* adding new rule to eslint config project

* updating react scripts eslint version

* Pinning version.

* Changing getter-return to warn

* Update package.json

* Update .eslintrc
Timer pushed a commit to Timer/create-react-app that referenced this pull request Jan 15, 2018
@chrislaughlin @Timer
Enable getter-return rule as a warning (facebook#3723)
f6c3730 
* Updating ESlint to ^4.15.0 and adding new rules to config

* remoning style rule and auto fixing breakages from new rules

* Removing implicit-arrow-linebreak style rule

* adding new rule to eslint config project

* updating react scripts eslint version

* Pinning version.

* Changing getter-return to warn

* Update package.json

* Update .eslintrc
@gaearon gaearon mentioned this pull request Jan 20, 2018
Closed
akstuhl pushed a commit to akstuhl/create-react-app that referenced this pull request Mar 15, 2018
@chrislaughlin @akstuhl
Enable getter-return rule as a warning (facebook#3723)
6a2ab79 
* Updating ESlint to ^4.15.0 and adding new rules to config

* remoning style rule and auto fixing breakages from new rules

* Removing implicit-arrow-linebreak style rule

* adding new rule to eslint config project

* updating react scripts eslint version

* Pinning version.

* Changing getter-return to warn

* Update package.json

* Update .eslintrc
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
@chrislaughlin @Timer
Enable getter-return rule as a warning (facebook#3723)
5a55208 
* Updating ESlint to ^4.15.0 and adding new rules to config

* remoning style rule and auto fixing breakages from new rules

* Removing implicit-arrow-linebreak style rule

* adding new rule to eslint config project

* updating react scripts eslint version

* Pinning version.

* Changing getter-return to warn

* Update package.json

* Update .eslintrc
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gaearon gaearon gaearon left review comments

Assignees
No one assigned
Labels
CLA Signed tag: internal
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@chrislaughlin @facebook-github-bot @gaearon @Timer @claughlin-r7