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.

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

Upgrade ESLint to 5.12.0 #11095

Merged
merged 16 commits into from Mar 18, 2019
Merged

Upgrade ESLint to 5.12.0 #11095

merged 16 commits into from Mar 18, 2019

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Jan 7, 2019

ESLint upgrade to latest version. Needed mainly to move Babel upgrade in #10810 forward. Old ESLint uses old version of the babel-eslint package that's not compatible with Babel 7.

There is plenty of new ESLint errors reported now, as the rules got upgraded:

  • instantiating functions inside connect -- leads to unnecessary rerenders
  • JSX a11y warnings
  • warnings about legacy React lifecycle methods
  • duplicate ES module imports
  • legacy string refs in React

I'll leave it up to the Jetpack team to decide whether they want to fix the issues or disable the offending rules.

@jetpackbot
Copy link

jetpackbot commented Jan 7, 2019

Warnings
⚠️ "Testing instructions" are missing for this PR. Please add some
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 9f25203

@jeherve jeherve requested a review from brbrr January 7, 2019 14:30
@jeherve
Copy link
Member

jeherve commented Jan 7, 2019

Related:

#10822

And further changes:
#10823
#10825
#10827

@jsnajdr
Copy link
Member Author

jsnajdr commented Jan 7, 2019

OK all these PRs seems to do the same thing. That's a good sign 😄 What are the next steps (cc @brbrr). Can I help with something?

@brbrr brbrr self-assigned this Jan 10, 2019
@brbrr
Copy link
Contributor

brbrr commented Jan 10, 2019

I'll be happy to take it over :)

@brbrr brbrr added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Jan 14, 2019
@brbrr
Copy link
Contributor

brbrr commented Jan 14, 2019

I went through all the ESLint errors and tried to fix all of them. Unfortunately, some of the errors are trickier than other errors, so I disabled some rules with the intention to tackle these errors later with a more incremental approach: #11142

Most rule fixes are grouped by commits for ease of review (you might not want to review the all the changes at once)

@kraftbj
Copy link
Contributor

kraftbj commented Mar 11, 2019

@brbrr I took a look and everything made sense to me. Would you be game for a rebase and we can get it across the line ASAP after the rebase.

@kraftbj kraftbj added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Mar 11, 2019
@brbrr brbrr added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Mar 18, 2019
@brbrr
Copy link
Contributor

brbrr commented Mar 18, 2019

Rebased! Let's push it through :)

@brbrr brbrr requested a review from kraftbj March 18, 2019 10:08
@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Mar 18, 2019
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

Code changes make sense to me. Built fine and the dashboard works as expected with changing settings, etc.

@kraftbj kraftbj merged commit 36c77f7 into master Mar 18, 2019
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 18, 2019
@kraftbj kraftbj deleted the upgrade/eslint-5 branch March 18, 2019 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants