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

Run ESLint over our JS files, fix all lints #50172

Merged
merged 3 commits into from
Aug 15, 2022

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Aug 3, 2022

In prep for adding copyright headers to all of our source files, make our lint task run ESLint on our JS files too (which will need the headers, and I want ESLint to enforce that).

Our .eslintignore already covers ignoring the js files we don't care about, e.g. build artifacts and baselines.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Aug 3, 2022
@jakebailey
Copy link
Member Author

I'm still waiting on guidance as to the copyright comments, but regardless I think this is a good change as it fixes a load of lint problems and type errors in our scripts.

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

Seems fine. How long does it take?

@jakebailey
Copy link
Member Author

That's a great question. It's 4x faster for some reason, which then made me notice that the gulp task is not working properly without the flag because eslint doesn't scan anything other than .js files by default, and it's typically the typescript-eslint recommended config that changes that. Oops. Will fix.

@jakebailey
Copy link
Member Author

Hm, no, ts-eslint's config doesn't do this. I don't know who does; if use ESLint on my own projects, it does the right thing, but it does the wrong thing on the TS repo.

@jakebailey
Copy link
Member Author

The reality is weirder; apparently the ESLint CLI will look at .js files and files that happen to be a part of an overrides config. Most tseslint users never know that ESLint normally only looks at .js files at the CLI because they extend the recommended config, which just so happens to override a couple of default rules.

@jakebailey
Copy link
Member Author

So, to re-answer with the fix, the new lint time is maybe about 1s longer than the old one, give or take, but it's already a 40s run uncached.

Cached, it's about 3s on my machine before/after.

@jakebailey jakebailey merged commit 9f7c0cb into microsoft:main Aug 15, 2022
@jakebailey jakebailey deleted the eslint-js branch August 15, 2022 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants