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

chore: switch from tslint to eslint #3651

Merged
merged 27 commits into from
Feb 2, 2023
Merged

chore: switch from tslint to eslint #3651

merged 27 commits into from
Feb 2, 2023

Conversation

Walther
Copy link
Contributor

@Walther Walther commented Feb 1, 2023

What this PR does / why we need it:

Switches from the deprecated TSLint to ESLint in our main repository.

Which issue(s) this PR fixes:

Fixes #3648

Special notes for your reviewer:

We should do another pass for simplifying and combining the various .esllintrc.js files into one good centralized configuration. This is just the initial pass to get the switch done. Edit: done ✅

I'm temporarily making max line width 120 only a warning, as our codebase has quite a lot of those to fix, if we want to enforce that. We can switch back to error level on those later, if we want. Edit: I loosened the max line length rule to not apply to comments, strings and template strings. We can tighten this later if we want.

⚠️ There are also a couple of lint errors that we should fix - a bunch of unawaited promises and some variable shadowing issues. Fixing some of these might affect behavior? I'm not sure. Edit: turned these into warnings as opposed to hard errors. Fix later.

Squash before merging.

@Orzelius
Copy link
Contributor

Orzelius commented Feb 1, 2023

I thought we pushed this into the future as it will make syncing changes with the 0.12 harder? Looking at the changes it doesn't seem to be that big of a deal though.

@vvagaytsev
Copy link
Collaborator

It looks like there are some conflicts.

@Walther
Copy link
Contributor Author

Walther commented Feb 1, 2023

I thought we pushed this into the future as it will make syncing changes with the 0.12 harder? Looking at the changes it doesn't seem to be that big of a deal though.

Sorry, I got distracted and tried my hand at this to see how much work it would be, and apparently it was not a big change after all. The changes required were all fairly minimal, so I feel like there should be fairly minimal risk.

It looks like there are some conflicts.

Thanks for noticing! My base branch wasn't quite up to date, so there was one conflict in test/unit/src/plugins/container/container.ts. Fixed it.


Of course if we feel like this is not the right time for this PR, we can do this again some other time. Key steps for this project:

  • find each directory that has a tslint.json file
  • run npx tslint-to-eslint-config --prettier --comments
  • edit package.json to remove references to tslint
  • edit the generated .eslintrc.json to remove references to tslint
  • edit package.json to change the lint script incantation
  • run yarn run lint, optionally with --fix
  • repeat for all relevant directories
  • done!

@edvald
Copy link
Collaborator

edvald commented Feb 1, 2023

Nice! Thanks for taking this on :) I think now is a great time for this, very good to shed more of deprecated packages from the repo and improve our DC along the way.

@vvagaytsev vvagaytsev added the 0.13 label Feb 2, 2023
@Walther
Copy link
Contributor Author

Walther commented Feb 2, 2023

check-docs fails, apparently this helm user guide has been removed from docs - but unrelated to this pr

@TimBeyer
Copy link
Contributor

TimBeyer commented Feb 2, 2023

I think this is great and should really not cause a lot of conflict pain later one.
One thing I wanted to still suggest is to have a separate .eslintrc.autogenerated.js and then inherit from that in .eslintrc.js and put our overrides in there.
That way it's easy to see what is autogenerated and what we chose explicitly, plus we can add comments to the rules that we may only want to temporarily disable. Then later on we can step by step move the rules we want to keep over to the .eslintrc.js and eventually throw away the autogenerated one.

@Walther Walther self-assigned this Feb 2, 2023
@Walther Walther merged commit febafae into 0.13 Feb 2, 2023
@Walther Walther deleted the tslint-to-eslint branch February 2, 2023 17:15
This was referenced May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants