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

Bump Style Rules and Linting #2084

Merged
merged 9 commits into from Jan 4, 2021
Merged

Bump Style Rules and Linting #2084

merged 9 commits into from Jan 4, 2021

Conversation

AdrianAndersen
Copy link
Member

@AdrianAndersen AdrianAndersen commented Dec 30, 2020

Changelog

  • Bump stylelint and prettier (No drama 😉 )
  • Bump eslint to newest version (aka. a lot of new rules:)
    • "==" and "!=" must be moved to "===" and "!=="
      • This caused issues in the previous PR 💀, but should be working now.
    • All switch statements needs a default:-tag (ez fix)
    • Flow type definitions needed to be sorted a bit
      • 2 places had circular defined types (aka. two types using eachother) I added this to eslint-ignore as this will be fixed when we move to Typescript.
    • allowed.hasOwnProperty(key) => Object.prototype.hasOwnProperty.call(allowed, key) (improves secuirty)
    • Some places where the return value of map() were unused needed to be moved to forEach()
    • location and confirm() are restricted globals, and had to be moved to window.location and ConfirmModalWithParent
    • Some places used () => (func1(), func2()) syntax, which had to be rewritten. I am a bit unsure about these rewrites, since I don't fully understand the aforementioned syntax. Hopefully someone can check that these are correct 🙂
  • Bumped eslint-config-react-app
    • Added eslint-plugin-react-hooks (required dependency to the new version)
    • Non named exports are no longer allowed, created a default export name instead
    • Had to pass a dependency array to useCallback(), passed [] to fix eslint warning
  • Bumps to other plugins (Not mentioned since these required no other changes)

Revert PR With Previous Review Threads

Future changes

As I worked on bumping and configuring eslint, I discovered that we have a quite poorly configured .eslintrc. There are many eslint plugins currently not in use, which limits eslint's benefits. In a future PR, I will configure some sane defaults for eslint, as well as enable default rules for the plugins we currently don't use. This will help improving the code quality of our codebase.

Copy link
Member

@odinuge odinuge left a comment

Choose a reason for hiding this comment

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

Awesome! Just a few nits, otherwise goood to go!

(Splitting of commits was awesome!)

app/reducers/events.js Outdated Show resolved Hide resolved
app/routes/admin/groups/components/GroupMembers.js Outdated Show resolved Hide resolved
app/routes/admin/groups/components/GroupMembers.js Outdated Show resolved Hide resolved
app/routes/admin/groups/components/GroupPermissions.js Outdated Show resolved Hide resolved
app/routes/admin/groups/components/GroupMembersList.js Outdated Show resolved Hide resolved
app/routes/admin/groups/components/GroupPermissions.js Outdated Show resolved Hide resolved
app/routes/surveys/components/SurveyEditor/SurveyEditor.js Outdated Show resolved Hide resolved
Copy link
Member

@odinuge odinuge left a comment

Choose a reason for hiding this comment

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

lgtm
:shipit:

@AdrianAndersen AdrianAndersen merged commit 8909b29 into master Jan 4, 2021
@AdrianAndersen AdrianAndersen deleted the bumpStyleAndLinting branch January 4, 2021 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants