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

Updating eslint config #1454

Draft
wants to merge 31 commits into
base: development
Choose a base branch
from
Draft

Updating eslint config #1454

wants to merge 31 commits into from

Conversation

joshsadam
Copy link
Contributor

@joshsadam joshsadam commented Jan 25, 2023

This is an excellent synopsis of why does JavaScript need a style guide from AirBnB

Why does JavaScript need a style guide?
Let's say you start an e-commerce site.

Your first engineer is Picasso during his blue period. Your codebase quickly becomes filled with Picasso blue period javascript files. He paints you a javascript shopping cart without a problem. He's done it a million times before. But this is his best one. It's his masterpiece. And very blue.

Your second engineer is Salvador Dali. He shows up on his first day with a camera, a single paintbrush, a silly mustache, and starts contributing some crazy surrealist javascript files.

This influences Picasso as they work together and Picasso starts switching things up and begins adding cubism files to the codebase. The codebase is so new that Picasso and Dali can continue adding new files at any time with no harm, because there is no maintenance when it's just two engineers adding new functionality. No one is stepping on each other's toes. There's enough paint for everyone.

Then you hire Monet and he comes in and leaves his impression on everything.

Now you have a blue period shopping cart, impressionist image lazy loading, surrealist photo slide show, and cubism style event tracking in /app/assets/javascripts. The javascripts folder becomes a museum.

Let's jump to the future 4 years later. Lots of major functionality has already been built. Picasso has left the company to pursue a company built on a guernica style. Your business focus has shifted a bit and you need someone to extend Picasso's blue period shopping cart to support collaborative shopping (lots of people sharing the same cart in realtime).

Luckily you just hired a rising star in javascript land that likes to spray paint things on the wall, his name is Banksy. So you tell Banksy, "Hey for your first job we need you to modify our shopping cart so it supports collaboration. Timing is really important for the big relaunch, fortunately this should be really easy because we already have Picasso's shopping cart and it's already a masterpiece. Just reuse that."

So Banksy goes in and spray paints over your Picasso shopping cart and makes it collaborative and you have a big successful relaunch.

A month later a bug report comes in about removing items from the shopping cart. Banksy is busy on another project, so you get Monet to go in and fix it.

Monet doesn't know how to use spray paint cans. So his attempts to fix it are sloppy. He breaks the build. He switches back to the brush and just paints over it, but it took a lot longer than it should have because of the time to understand and adopt an unfamiliar style.

When you have over 80 engineers contributing to one codebase, you quickly learn your usual ways of doing things don't work. So we try to turn everyone into Picasso. No matter where you jump in to the codebase, all the files are familiar and look like they were painted by you.

The most important thing, no matter what your preferred javascript style is, is to be consistent when working with a team or a large codebase that will have to be maintained in the future.

The style that works best for our team is our Picasso style since that's how it all started.

We open sourced our style guide so other teams could fork it and turn it into a Monet style guide or a Banksy style guide. Which is lots of fun to watch.

As an individual painter/engineer working on side projects and exploring all the wonderful things you can do with javascript, please throw conventions away and ignore everything anyone has ever said.

It's the only way the world will enjoy the next Picasso.

This PR will cause lots of lining errors once it is in so please review it carefully.

Summarized this stuff in the WikI:

Updated to using AirBnB eslint typescript config. This is a much more strict configuration than we are currently using so it will pick up a lot of things. We still will need to turn on other options as we come across them, but this will definitely provide a solid and tested base.

  • Please review all AirBnB JavaScript Style Guide. Reading through this will give you an idea of what is expected in your code. Using a predefined style guide instead of our own means we will not need to update a style guide document.
  • Please make sure the Prettier works with your selected IDE with this update.
  • Please test it out in as many ways as you need. Run ./gradlew lintWebapp from the root directory so you can see the output.
  • Please review Eslint Rules (https://typescript-eslint.io/rules/) to see if there is anything that is not included that you want to be covered

Checklist

  • CHANGELOG.md (and UPGRADING.md if necessary) updated with information for new change.
  • Tests added (or description of how to test) for any new features.

@joshsadam joshsadam marked this pull request as ready for review January 26, 2023 16:11
@joshsadam joshsadam added UI User Interface Issue developer Issues effecting developers of IRIDA, but that aren't user facing dependencies Pull requests that update a dependency file labels Jan 26, 2023
Copy link
Member

@ericenns ericenns left a comment

Choose a reason for hiding this comment

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

I am getting errors running ./graldew lintWebapp.

[irida-ui lint] /home/eenns/irida/src/main/webapp/resources/js/pages/projects/linelist/components/LineList/LineListLayoutComponent.jsx
[irida-ui lint]   0:0  error  Parsing error: ESLint was configured to run on `<tsconfigRootDir>/resources/js/pages/projects/linelist/components/LineList/LineListLayoutComponent.jsx` using `parserOptions.project`: <tsconfigRootDir>/tsconfig.json
[irida-ui lint] However, that TSConfig does not include this file. Either:
[irida-ui lint] - Change ESLint's list of included files to not include this file
[irida-ui lint] - Change that TSConfig to include this file
[irida-ui lint] - Create a new TSConfig that includes this file and include it in your parserOptions.project
[irida-ui lint] See the typescript-eslint docs for more info: https://typescript-eslint.io/linting/troubleshooting#i-get-errors-telling-me-eslint-was-configured-to-run--however-that-tsconfig-does-not--none-of-those-tsconfigs-include-this-file

build.gradle.kts Show resolved Hide resolved
@joshsadam
Copy link
Contributor Author

joshsadam commented Jan 27, 2023

Adopt  Error Meaning Reference
✔️ @typescript-eslint/default-param-last Enforce default parameters to be last. https://typescript-eslint.io/rules/default-param-last
✔️  @typescript-eslint/naming-convention Enforce naming conventions for everything across a codebase. https://typescript-eslint.io/rules/naming-convention
 :heavy_check_mark: @typescript-eslint/no-explicit-any Disallow the any type. https://typescript-eslint.io/rules/no-explicit-any
✔️  @typescript-eslint/no-implied-eval Disallow the use of eval()-like methods. https://typescript-eslint.io/rules/no-implied-eval
✔️  @typescript-eslint/no-use-before-define Disallow the use of variables before they are defined. https://typescript-eslint.io/rules/no-use-before-define
✔️  @typescript-eslint/no-unused-expressions Disallow unused expressions. https://typescript-eslint.io/rules/no-unused-expressions
✔️  @typescript-eslint/no-unused-vars Disallow unused variables. https://typescript-eslint.io/rules/no-unused-vars
✔️  @typescript-eslint/return-await Enforce consistent returning of awaited values. https://typescript-eslint.io/rules/return-await
✔️  @typescript-eslint/no-shadow Disallow variable declarations from shadowing variables declared in the outer scope. https://typescript-eslint.io/rules/no-shadow/
✔️  Array-callback-return Enforce return statements in callbacks of array methods https://eslint.org/docs/latest/rules/array-callback-return
✔️  consistent-return Require return statements to either always or never specify values https://eslint.org/docs/latest/rules/consistent-return
✔️  eqeqeq Require the use of === and !== https://eslint.org/docs/latest/rules/eqeqeq
✔️  guard-for-in Looping over objects with a for in loop will include properties that are inherited through the prototype chain. This behavior can lead to unexpected items in your for loop. https://eslint.org/docs/latest/rules/guard-for-in
✔️  import/cycle Ensures that there is no resolvable path back to this module via its dependencies. https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-cycle.md
✔️  import/order Enforce a convention in the order of require() / import statements. +(fixable) The --fix option on the [command line] automatically fixes problems reported by this rule. https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md
✔️ import/named Verifies that all named imports are part of the set of named exports in the referenced module. https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/named.md
✔️  import/no-duplicates Disallow duplicate module imports https://eslint.org/docs/latest/rules/no-duplicate-imports
✔️  import/no-named-as-default Reports use of an exported name as the locally imported name of a default export. https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-named-as-default.md
✔️  import/prefer-default-export Prefer default export on a file with single export https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/prefer-default-export.md
✔️  no-console Disallow the use of console https://eslint.org/docs/latest/rules/no-console
✔️ no-else-return Disallow else blocks after return statements in if statements https://eslint.org/docs/latest/rules/no-else-return
✔️  no-empty-pattern Disallow empty destructuring patterns https://eslint.org/docs/latest/rules/no-empty-pattern
✔️  no-nested-ternary Disallow nested ternary expressions https://eslint.org/docs/latest/rules/no-nested-ternary
✔️  no-param-reassign This rule aims to prevent unintended behavior caused by modification or reassignment of function parameters. https://eslint.org/docs/latest/rules/no-param-reassign
✔️  No-plusplus Disallow the unary operators ++ and -- https://eslint.org/docs/latest/rules/no-plusplus
✔️  no-restricted-globals Explicitly call global variables (e.g. Number.isNan vs just isNaN) https://eslint.org/docs/latest/rules/no-restricted-globals
✔️ no-restricted-syntax This rule disallows specified (that is, user-defined) syntax. https://eslint.org/docs/latest/rules/no-restricted-syntax
✔️  no-return-assign Disallow assignment operators in return statements https://eslint.org/docs/latest/rules/no-return-assign
✔️ no-underscore-dangle Disallow dangling underscores in identifiers https://eslint.org/docs/latest/rules/no-underscore-dangle
✔️  Object-shorthand Require or disallow method and property shorthand syntax for object literals https://eslint.org/docs/latest/rules/object-shorthand
✔️  one-var Enforce variables to be declared either together or separately in functions https://eslint.org/docs/latest/rules/one-var
✔️  Prefer-const Require const declarations for variables that are never reassigned after delcared https://eslint.org/docs/latest/rules/prefer-const
✔️  Prefer-promise-reject-errors Require using Error objects as Promise rejection reasons https://eslint.org/docs/latest/rules/prefer-promise-reject-errors
✔️  Prefer-regex-literals Disallow use of the RegExp constructor in favor of regular expression literals https://eslint.org/docs/latest/rules/prefer-regex-literals
✔️  prefer-template Require template literals instead of string concatenation https://eslint.org/docs/latest/rules/prefer-template
✔️  radix Enforce the consistent use of the radix argument when using parseInt() https://eslint.org/docs/latest/rules/radix
✔️  react/jsx-boolean-value This rule will enforce one or the other to keep consistency in your code. https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-boolean-value.md
✔️  react/function-component-definition This option enforces a specific function type for function components. https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/function-component-definition.md
✔️  react/jsx-curly-brace-presence Disallow unnecessary JSX expressions when literals alone are sufficient https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-curly-brace-presence.md
✔️  react/jsx-filename-extension JSX not allowed in files with extension '.js' https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-filename-extension.md
✔️  react/no-array-index-key Disallow usage of Array index in keys https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-array-index-key.md
✔️  react/no-unstable-nested-components Creating components inside components (nested components) will cause React to throw away the state of those nested components on each re-render of their parent. https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-unstable-nested-components.md
✔️  react/jsx-no-useless-fragment A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment. https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-no-useless-fragment.md
✔️ react/jsx-props-no-spreading Enforces that there is no spreading for any JSX attribute. This enhances readability of code by being more explicit about what props are received by the component. It is also good for maintainability by avoiding passing unintentional extra props and allowing react to emit warnings when invalid HTML props are passed to HTML elements. https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-props-no-spreading.md
✔️ react/no-danger Dangerous properties in React are those whose behavior is known to be a common source of application vulnerabilities. The properties names clearly indicate they are dangerous and should be avoided unless great care is taken. https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-danger.md

@joshsadam
Copy link
Contributor Author

For the createRedcuer code blockes where the state param gets reassigment, we need to add:

/* eslint-disable no-param-reassign */

on the first line inside the builderCallback

@joshsadam
Copy link
Contributor Author

joshsadam commented Jan 30, 2023

I want to add:

consistent-type-imports typescript-eslint

TypeScript allows specifying a type keyword on imports to indicate that the export exists only in the type system, not at runtime. This allows transpilers to drop imports without knowing the types of the dependencies.

"@typescript-eslint/consistent-type-imports": "warn"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file developer Issues effecting developers of IRIDA, but that aren't user facing UI User Interface Issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants