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: run eslint with type information on CI #13368

Merged
merged 13 commits into from Oct 3, 2022
Merged

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Oct 3, 2022

Summary

We OOM if we run the entire lint run with type info (see https://typescript-eslint.io/docs/linting/typed-linting/monorepos/#important-note-regarding-large--10-multi-package-monorepos), so I've added a script.

We have a toooooon of errors, so I just fixed a few of them. Should chip away at this when people have time 🙂

This is somewhat annoying as IDE integrations etc won't apply, but better than nothing

Test plan

Added CI run

scripts/lintTs.mjs Outdated Show resolved Hide resolved
@@ -363,19 +362,16 @@ const makeThrowingMatcher = (
})();

if (isPromise(potentialResult)) {
const asyncResult = potentialResult as AsyncExpectationResult;
Copy link
Member Author

Choose a reason for hiding this comment

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

only change to non-test code. The cast was unnecessary as TS could infer it from it being a promise

@@ -114,10 +114,10 @@ const IDVisitor = {
],
};

const FUNCTIONS: Record<
const FUNCTIONS = Object.create(null) as Record<
Copy link
Member Author

Choose a reason for hiding this comment

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

Object.create returns any

Comment on lines +26 to +29
'babel-jest',
'babel-plugin-jest-hoist',
'diff-sequences',
'jest-source-map',
Copy link
Contributor

Choose a reason for hiding this comment

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

Only four packages? That’s not su bad. It is indeed good idea to have this added!

Copy link
Member Author

Choose a reason for hiding this comment

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

this is a list of which we lint, not the ones excluded. running on e.g. expect gives almost 400 errors, so I stopped there 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah.. So that’s a good start then ;D

loadPartialConfig: jest.fn((...args) => actual.loadPartialConfig(...args)),
loadPartialConfigAsync: jest.fn((...args) =>
actual.loadPartialConfigAsync(...args),
loadPartialConfig: jest.fn<typeof actual.loadPartialConfig>((...args) =>
Copy link
Member Author

Choose a reason for hiding this comment

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

#13367 came from this 🙂

@SimenB
Copy link
Member Author

SimenB commented Oct 3, 2022

Current state running against all packages btw: ✖ 2979 problems (2979 errors, 0 warnings)

@SimenB SimenB merged commit 50bab21 into jestjs:main Oct 3, 2022
@SimenB SimenB deleted the lint-ts branch October 3, 2022 12:55
@github-actions
Copy link

github-actions bot commented Nov 3, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants