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

Migrate to ESLint #1944

Merged
merged 26 commits into from
Sep 11, 2024
Merged

Migrate to ESLint #1944

merged 26 commits into from
Sep 11, 2024

Conversation

nagilson
Copy link
Member

@nagilson nagilson commented Sep 9, 2024

Summary

TSLint was deprecated when this project had no maintainer.
We waited a while to migrate because there was no real reason to.

I followed https://code.visualstudio.com/api/advanced-topics/tslint-eslint-migration to help migrate.

Nowadays TSLint is failing a lot in our pipeline and having sporadic behavior. This might be due to upgrading to node 20. Or maybe due to the .editorconfig. I figured now was the time to migrate.

This introduced about 900 new linting errors. I fixed some of them. For files with no code churn, I disabled the linter for those files. If/when we touch those files again, we can fix those issues then. Some of them are very time consuming and don't make sense to test unless we are editing the pre-existing behavior because it's already a 'known-good.'

Linting Issues

One of the errors I did add a disable on a per line basis was unsafe member access. Unfortunately this only checks against calling . on the any type, not whether accessing .foo is safe to call (e.g. x?.bar is OK, but not x.bar when x is any and could this be undefined or null.)

But I didnt want to disable it project wide because without the ? it is actually a serious problem. But in JS anything can be thrown (not just exception) so we want to catch it all. But that makes eslint very mad.

So I resorted to commenting on it everywhere in the code which is ugly, but there doesn't seem to be a better option for now, unless we want to only catch errors, but some libraries will throw stuff up that's not an error because yay javascript
https://stackoverflow.com/questions/66462386/how-to-handle-the-typescript-eslint-no-unsafe-member-access-rule-on-catcherr

Why ES Lint 8

There is a thing called ES Lint 9. That seems to not really support migrating from an eslintrc.js file. And that is the file type that's created when you want to convert from a tslint project to lint typescript. Even their tooling says dont do this yet.

@nagilson nagilson enabled auto-merge (squash) September 10, 2024 22:48
@nagilson nagilson requested a review from a team September 10, 2024 22:48
@nagilson nagilson merged commit e9c440e into dotnet:main Sep 11, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants