[New] add TypeScript types #3097
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3097 +/- ##
===========================================
+ Coverage 81.99% 95.46% +13.46%
===========================================
Files 94 83 -11
Lines 4154 3570 -584
Branches 1395 1247 -148
===========================================
+ Hits 3406 3408 +2
+ Misses 748 162 -586 ☔ View full report in Codecov by Sentry. |
|
Can we add a tsc run, at least? |
| }; | ||
| rules: { | ||
| [key: string]: ESLint.Rule.RuleModule; | ||
| }; |
There was a problem hiding this comment.
We could also add types for settings, something like:
settings?: Record<string, unknown> & {
// we can add here every known property
'import/resolver'?: Record<string, unknown> &
{ typescript?: { alwaysTryTypes?: boolean } }
'import/extensions'?: ...
}There was a problem hiding this comment.
Yup in the long-run that'd be ideal and I'm happy to help work on that but I'd prefer to not block this on that since I'm not super familiar with all the settings and really right now their types won't get used out of the box since the new config system doesn't result in the types getting automatically applied.
i.e. they'd be useful for letting you manually type an object via pluginImport['settings'] but that's a more complex setup
There was a problem hiding this comment.
(I actually only happened to include rules at all as I had that type on hand from what we've got in eslint-plugin-jest - you could make the same point that ideally the rules could be typed with all their settings, but again that'd be a lot of work for something that right now wouldn't be that usable)
Done - note that it requires using |
| @@ -34,6 +36,7 @@ | |||
| "test-examples": "npm run build && npm run test-example:legacy && npm run test-example:flat", | |||
| "test-example:legacy": "cd examples/legacy && npm install && npm run lint", | |||
| "test-example:flat": "cd examples/flat && npm install && npm run lint", | |||
| "test-types": "npx --package typescript@latest tsc --noEmit index.d.ts", | |||
There was a problem hiding this comment.
shouldn't noEmit be set in tsconfig, and tsc be run on the whole project?
also, let's add typescript to the dev deps rather than using npx.
There was a problem hiding this comment.
The issue is typescripts already a dev dependency but supporting versions from 2 through to 4, whereas this check has to use v5 because @types requires that as a min. version and I assume that the older versions are required for a reason.
I didn't create a tsconfig.json as that will be picked up by any running of typescript, and having it defined can change other aspects of linting such as what files are included by default.
There was a problem hiding this comment.
shouldn't noEmit be set in tsconfig, and tsc be run on the whole project?
That'd mean the whole codebase would get type checked, requiring it to be correctly typed - while I personally would support that, I think it's out of scope for this pr
There was a problem hiding this comment.
So I did have a go at running tsc across the whole codebase just to see how much we'd get, and well with strict mode its 3498 errors and without it its 857 😅
Technically, we could have a tsconfig.json that includes just the .d.ts (and even .js because checkJs is false by default), but it feels weird to me having that as that only config option and for just a single small file...
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is protestware?This package is a joke, parody, or includes undocumented or hidden behavior unrelated to its primary function. Consider that consuming this package may come along with functionality unrelated to its primary purpose. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
For now I've just typed these out manually based on what we're using internally - while it would be good to have some tests at some point, I'm hoping/recommending not blocking on that for now as I think the types are small enough to be easy to maintain manually for the time being.
Resolves #3090