Skip to content

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
Collaborator

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 This PR is ready for review. 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 need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Mar 11, 2019
@brbrr brbrr force-pushed the upgrade/eslint-5 branch from 0719109 to 9f25203 Compare March 18, 2019 09:56
@brbrr brbrr added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. 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 This PR is ready for review. 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.

7 participants