refactor: strict typing and deprecation fix in type definitions

jest-community / eslint-plugin-jest

ESLint plugin for Jest

MIT License

1.12k stars 229 forks source link

refactor: strict typing and deprecation fix in type definitions #1573

Closed y-hsgw closed 2 months ago

y-hsgw commented 2 months ago

I apologize for the sudden PR, but I'd like to propose some strict typing enhancements. Please consider the following changes:

Updated deprecated type definitions for TSESLint.Linter.Config.
Removed unnecessary type assertions and enhanced strict typing.

Note: As this is the first PR for this repository, please feel free to provide any feedback or suggestions if there are any deficiencies.

G-Rath commented 2 months ago

I'll review this in more detail tomorrow when I'm at my laptop, but I don't see anything here that I'd call strictly better - can you describe what problem you're seeing that you're trying to solve?

y-hsgw commented 2 months ago

I'll review this in more detail tomorrow when I'm at my laptop, but I don't see anything here that I'd call strictly better - can you describe what problem you're seeing that you're trying to solve?

Here are the two points:

Updated due to deprecation of TSESLint.Linter.Config.
Removed unnecessary type assertions. I would suggest that type assertions should be avoided as much as possible from a conservatism standpoint.

I submitted the proposed changes with the hope of contributing in any way possible. There might be some areas where I lack understanding, so I would greatly appreciate any feedback or guidance you could provide 🙇‍♂️

G-Rath commented 2 months ago

But you're not removing any unneeded type assertions? I appreciate your enthusiasm but I'm not seeing anything here that I'd say is a strong improvement and worth the git churn.

The deprecated type is deliberate for now as we're in the middle of supporting both legacy and flat config, and different versions of @typescript-eslint (which still don't support ESLint v9 officially, so they might have further type changes for that) - I've got a branch locally for this that I'm hoping to get pushed up in the next couple of days.

Since we currently don't publish types, none of this should impact downstream consumers either

y-hsgw commented 2 months ago

Thank you for your explanation. Indeed, as you mentioned, this PR was a minor adjustment. I will close this PR now. You have my apologies for any inconvenience caused 🙇‍♂️

G-Rath commented 2 months ago

No worries - as I said I appreciate the enthusiasm; feel free to have a look over some of our issues and have a go at tackling one of them if you want to help out :)

G-Rath commented 2 months ago

@y-hsgw just catching up on my general todo list of which re-reviewing this was one as I think there are a couple of interesting possible changes highlighted (#1577 being one).

It does look like some of our as const's are not needed - I'm not sure if that's because TypeScript has gotten better, the types have changed, or there's some footgun awaiting us, but if you want to do a refactoring PR that only removes as consts that don't result in type errors, that'd be cool.

y-hsgw commented 2 months ago

It does look like some of our as const's are not needed - I'm not sure if that's because TypeScript has gotten better, the types have changed, or there's some footgun awaiting us, but if you want to do a refactoring PR that only removes as consts that don't result in type errors, that'd be cool.

@G-Rath Thanks! I've created a PR at https://github.com/jest-community/eslint-plugin-jest/pull/1578! Please review it.