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

refactor(deps): migrate from deprecated tslint to eslint #12163

Merged

Commits on Nov 25, 2023

  1. refactor(deps): migrate from deprecated tslint to eslint

    - `tslint` had been officially deprecated in favor of `eslint` with `@typescript-eslint` since at least [early 2019](https://blog.palantir.com/tslint-in-2019-1a144c2317a9)
      - it's been completely [archived](https://github.com/palantir/tslint) since at least mid-2021 as well
    
    - modify static analysis docs to mention `eslint` instead of `tslint`
      - also update note about Snyk to include dependency scans and mention that it's used as an SCA (Software Composition Analysis)
    
    - migrate `lint` script to use `eslint`
    - migrate all `tslint` deps, including `plugin-react` and `plugin-prettier`
      - **NOTE**: we have an outdated version of `prettier` (v1.x, while v3.x is latest), so equivalently had to use an outdated version of `eslint-plugin-prettier`
        - I imagine this might have been outdated because there was no newer `tslint-plugin-prettier` that supported newer `prettier` versions
          - i.e. this very `eslint` migration was a pre-requisite to upgrade `prettier`
    - migrate `tslint.json` to `.eslintrc.json`
      - more or less the same configuration with same plugins
      - use recommended rulesets
      - globally disable `@typescript-eslint/no-explicit-any` and `@typescript-eslint/no-non-null-assertion`
        - these are commonly disabled as both of these are already _explicit_ typings (as opposed to implicit ones, which we should check for)
          - there are valid uses of these two, and while they can be misused, that's somewhat uncommon (in my experience)
        - consistent with `argo-ui`'s configuration
    
    - auto-fix `<a target='_blank'>` elements that were missing a `rel='noreferrer'`, per [`react/jsx-no-target-blank` rule](https://github.com/jsx-eslint/eslint-plugin-react/blob/v7.33.2/docs/rules/jsx-no-target-blank.md)
      - this is a security vuln per [the docs](https://github.com/jsx-eslint/eslint-plugin-react/blob/ca30f77196d410c7cdb1e4fe11f11bfffb46c84f/docs/rules/jsx-no-target-blank.md?plain=1#L9)
        - see also https://mathiasbynens.github.io/rel-noopener and https://html.spec.whatwg.org/multipage/links.html#link-type-noopener
          - browsers have implicitly added `rel='noreferrer'` since at least 2021 as well per the above spec change; this (recommended) rule makes it explicitly added
    
    - auto-fix some unnecessary `!!`, per [`no-extra-boolean-cast` rule](https://eslint.org/docs/latest/rules/no-extra-boolean-cast)
    
    - auto-format `webpack.config.js` as previously `prettier` wasn't run on it (not a TS file)
      - manually add some ESLint comments so that it works with Node & CJS
        - may want to migrate to `webpack.config.mjs` in the future for ESM
          - or [`webpack.config.ts`](https://webpack.js.org/configuration/configuration-languages/#typescript)
    
    - manually fix several unused variables, per [`@typescript-eslint/no-unused-vars` rule](https://typescript-eslint.io/rules/no-unused-vars)
      - remove unused `_`, `e`, `x`, `i`, etc
      - remove unused `onError` prop
      - actually use the previously unused `T` generic type in `object-parser`
    
    - manually fix unnecessary `<T extends any>`, per [`@typescript-eslint/no-unnecessary-type-constraint` rule](https://typescript-eslint.io/rules/no-unnecessary-type-constraint)
      - just use regular `<T>`, which is the same
      - editor had some trouble understanding the syntax, so while doing so, also converted several consts assigned to anonymous functions to named functions
        - and then had a few more inner changes to named functions while at it too
          - and one promise chain -> async/await refactor while at that as well
    
    - manually fix two missing `key`s in React iterators, per [`react/jsx-key` rule](https://github.com/jsx-eslint/eslint-plugin-react/blob/v7.33.2/docs/rules/jsx-key.md)
      - React warns when these are missing at run-time as well, see [React docs on `key`](https://react.dev/learn/rendering-lists#keeping-list-items-in-order-with-key)
    
    - manually replace several single and double quotes inside of JSX with HTML escaped chars instead, per  [`react/no-unescaped-entities` rule](https://github.com/jsx-eslint/eslint-plugin-react/blob/v7.33.2/docs/rules/no-unescaped-entities.md)
      - use `&apos;` and `&quot;` consistently for these
    
    - manually fix a few `require` statements, per [`@typescript-eslint/no-var-requires` rule](https://typescript-eslint.io/rules/no-var-requires/)
      - disable in `webpack.config.js` per above
      - disable in `index.tsx` as I'm not sure if `react-hot-loader` can support async imports
        - `react-hot-loader` is also [deprecated in favor of React Fast Refresh](https://github.com/gaearon/react-hot-loader#moving-towards-next-step) and hasn't received an update in at least a year
          - so eventually we should replace it anyway. rather than trying to get it to work with async imports, just add a disable line
      - remove and uninstall `superagent-promise` as it's not needed nowadays
        - `superagent-promise` hasn't gotten an update in at least 5 years and `superagent` has long natively supported promises
          - heck the types used were from `superagent` itself also
      - convert a few `require('*.scss')` side-effects into `import '*.scss'`
        - `react-datepicker` already had a `.css` `import` as well
        - consistently use ESM syntax where possible
        - also, re-group some imports while at it
          - consistently have external imports -> internal imports -> internal side-effects (e.g. CSS)
    
    - manually remove unnecessary `\` escape chars, per [`no-useless-escape` rule](https://eslint.org/docs/latest/rules/no-useless-escape)
    - manually remove an unnecessary `// @ts-ignore` comment, per [`@typescript-eslint/ban-ts-comment` rule](https://typescript-eslint.io/rules/ban-ts-comment)
      - it should be replaced with a `// @ts-expect-error` instead, but this seemed to be unnecessary as there was no error at this time (there may have been in the past?)
    - manually remove unused `qeId` prop, per [`react/no-unknown-property` rule](https://github.com/jsx-eslint/eslint-plugin-react/blob/v7.33.2/docs/rules/no-unknown-property.md)
      - this seems to have been for testing purposes, but it is unused and also outdated
        - current testing paradigms tend to use valid HTML [`data-*` attributes](https://developer.mozilla.org/en-US/docs/Learn/HTML/Howto/Use_data_attributes) nowadays, such as Testing Library's [`data-testid`](https://testing-library.com/docs/queries/bytestid)
    
    - manually remove a few of the `tslint:disable` comments
      - `no-console`, `no-bitwise`, and `prefer-for-of`
        - `no-console` and `no-bitwise` are not part of ESLint's [recommended ruleset](https://eslint.org/docs/latest/rules/) at this time, so no need replace with `eslint-disable` comments
        - `prefer-for-of` is [part of](https://github.com/typescript-eslint/typescript-eslint/blob/75c128856b1ce05a4fec799bfa6de03b3dab03d0/packages/eslint-plugin/src/configs/stylistic.ts#L24) the separated `stylistic` package of `typescript-eslint`, which we do not currently use
    
    - also convert a few consts assigned to anonymous functions to named functions for better tracing while refactoring related code
    
    Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
    agilgur5 committed Nov 25, 2023
    Configuration menu
    Copy the full SHA
    e9a4a0e View commit details
    Browse the repository at this point in the history