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 config, fix errors #3125

Merged
merged 1 commit into from
Aug 28, 2020
Merged

Conversation

alicewriteswrongs
Copy link
Contributor

@alicewriteswrongs alicewriteswrongs commented Aug 26, 2020

Pre-Flight checklist

  • Migrations
    • Migration is backwards-compatible with current production code
  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

none

What's this PR do?

this just updates to the latest version of our eslint configuration, which adds the react-hooks preset. this allows the linter to find a lot of issues where the logic of hooks is being violated - it found a bunch of callbacks which did not have their dependencies properly specified, for instance, which could lead to bugs related to stale data inside the closure of the callback.

How should this be manually tested?

there aren't really any code changes - the only things are renaming some things so that the linter knows they are hooks and turning off rules in a few, specific places. so I think just read through it and make sure things look good, and possibly do a quick smoke test of the app to make sure I didn't break anything.

@codecov-commenter
Copy link

codecov-commenter commented Aug 26, 2020

Codecov Report

Merging #3125 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3125   +/-   ##
=======================================
  Coverage   88.52%   88.52%           
=======================================
  Files         246      246           
  Lines       10741    10741           
  Branches     1233     1233           
=======================================
  Hits         9509     9509           
  Misses       1056     1056           
  Partials      176      176           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67e2e2e...8d8f83b. Read the comment docs.

@@ -35,6 +35,9 @@ export function useLearningResourcePermalink() {
})
)
}
// dependencies intentionall blank here

Choose a reason for hiding this comment

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

intentionally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol

this bumps up to the latest version of our shared eslint configuration,
which adds an eslint package for linting react hooks. it found a few
issues in our code, so I fixed those to get the linter passing.

pr #3125
@alicewriteswrongs alicewriteswrongs merged commit 09fdfd6 into master Aug 28, 2020
@alicewriteswrongs alicewriteswrongs deleted the ap/upgrade-eslint branch August 28, 2020 13:31
@github-pages github-pages bot temporarily deployed to github-pages August 28, 2020 13:31 Inactive
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.

4 participants