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

Conversation

agilgur5
Copy link

@agilgur5 agilgur5 commented Nov 7, 2023

Motivation

Config is based on argoproj/argo-ui#265 (which I reviewed and which was written by a Prettier maintainer) and double-checked against another tslint -> eslint migration I recently reviewed as well in ezolenko/rollup-plugin-typescript2#463

Modifications

General migration changes

  • 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 (v3.x of the plugin, see v4 breaking changes and v5 breaking changes that require prettier v2 and v3, respectively)
      • 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 in chore: migrates from tslint to eslint argo-ui#265

Automatic fixes and changes

Manual fixes and changes

  • manually fix several unused variables, per @typescript-eslint/no-unused-vars rule

    • 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

    • 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
  • manually fix two missing keys in React iterators, per react/jsx-key rule

  • manually replace several single and double quotes inside of JSX with HTML escaped chars instead, per react/no-unescaped-entities rule

    • use &apos; and &quot; consistently for these
  • manually fix a few require statements, per @typescript-eslint/no-var-requires rule

    • 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 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

  • manually remove an unnecessary // @ts-ignore comment, per @typescript-eslint/ban-ts-comment rule

    • 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

    • this seems to have been for testing purposes, but it is unused and also outdated
  • 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 at this time, so no need replace with eslint-disable comments
      • prefer-for-of is part of 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

Verification

Tested locally, had no issues. Screenshot of UI working:
Screenshot 2023-11-07 at 3 12 29 AM

It's also primarily build/CI and stylistic changes

@agilgur5 agilgur5 added type/dependencies PRs and issues specific to updating dependencies area/ui javascript Pull requests that update Javascript dependencies labels Nov 7, 2023
@agilgur5 agilgur5 force-pushed the refactor-deps-tslint-to-eslint branch from 045e891 to 1b1ea19 Compare November 7, 2023 09:06
@terrytangyuan terrytangyuan enabled auto-merge (squash) November 20, 2023 20:05
@terrytangyuan
Copy link
Member

Please resolve conflicts

- `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>
auto-merge was automatically disabled November 25, 2023 18:40

Head branch was pushed to by a user without write access

@agilgur5 agilgur5 force-pushed the refactor-deps-tslint-to-eslint branch from 1b1ea19 to e9a4a0e Compare November 25, 2023 18:40
@agilgur5
Copy link
Author

Rebased and resolved conflicts

@terrytangyuan terrytangyuan merged commit d7b49c8 into argoproj:master Nov 25, 2023
15 checks passed
@agilgur5 agilgur5 deleted the refactor-deps-tslint-to-eslint branch November 25, 2023 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui javascript Pull requests that update Javascript dependencies type/dependencies PRs and issues specific to updating dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants