Update TypeScript and ESLint to latest versions #1256
Conversation
Signed-off-by: Máté Szabó <mszabo@fandom.com>
mszabo-wikia
requested review from
everett980,
rubenvp8510 and
tiffon
as code owners
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1256 +/- ##
==========================================
+ Coverage 95.47% 95.57% +0.10%
==========================================
Files 244 244
Lines 7750 7752 +2
Branches 2017 2016 -1
==========================================
+ Hits 7399 7409 +10
+ Misses 351 343 -8
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| @@ -57,7 +56,16 @@ module.exports = { | |||
| prefix: ['I'], | |||
| }, | |||
| ], | |||
|
|
|||
| // Disable ESLint core rules for which @typescript-eslint provides TypeScript-specific equivalents. | |||
There was a problem hiding this comment.
Just curious, what's the harm in keeping them? if TS flags them first, then fixing the issue would also fix ESLint issue, would it not? (basically, I prefer fewer deviations from defaults if possible)
There was a problem hiding this comment.
The problem is that these can cause false positives with the new version of @typescript-eslint. E.g. removing the suppression for core's no-shadow rules results in a plethora of bogus reports about TS type definitions. The recommendation of silencing the corresponding core rules comes from the plugin itself.
There was a problem hiding this comment.
Another way to fix it could be to make the root eslintrc also extend the recommended config from @typescript-eslint, because that config already does this toggling for us. This is what plexus does already. I'm thinking this could be done in a followup PR as it might cause some new errors.
| @@ -111,6 +111,7 @@ export function getNodeRenderer({ | |||
| return function renderNode(vertex: TDdgVertex, _: unknown, lv: TLayoutVertex<any> | null) { | |||
| const { isFocalNode, key, operation, service } = vertex; | |||
| return ( | |||
| // eslint-disable-next-line @typescript-eslint/no-use-before-define | |||
There was a problem hiding this comment.
it's good to have a comment explaining the reason (I assume it's a false positive)
| }; | ||
|
|
||
| // menu controls the dropdown menu in the top-right corner of the UI. | ||
| // When populated, this element completely overrides the default menu. | ||
| menu: (ConfigMenuGroup | ConfigMenuItem)[]; | ||
| menu: readonly (ConfigMenuGroup | ConfigMenuItem)[]; |
There was a problem hiding this comment.
curious why these specific entries need readonly and not all others.
There was a problem hiding this comment.
It seems it's specifically for array type entries. TS is apparently now better at detecting that the object passed to deepFreeze is recursively readonly.
|
🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 |
|
Another awesome PR! It looks like it's going to unblock a lot of pending dependabot upgrades, several of them already turned green after failing CI for months. I'll be merging them today (unfortunately, every time I merge one I need to wait for the CI to run on all others, takes a lot of time). |
|
Awesome, that's great to see :) |
- Resolves jaegertracing#818 Update TypeScript, ESLint and `@typescript-eslint` to the latest versions. Notable changes: * Update the list of core ESLint rules that need to be disabled because `@typescript-eslint` provides TypeScript-specific equivalents for them, and change existing suppressions referencing the now-disabled core rules. Group these together in the ESLint config for clarity. * Fix references to unimported types in `serviceGraph.tsx` and `types.tsx`. * It seems that the type of `defaultConfig` and the user-defined config wasn't correctly inferred as `Config` before the upgrade. Fix various type errors resulting from this: * `'__mergeFields'`, i.e. the list of config options where user-specified values should be merged with the default object values in `defaultConfig`, was defined via `defineProperty` and so seemingly inferred as an unknown type. This now causes a type error when iterating over its keys to try and merge the relevant property values, as TypeScript can't guarantee that the property values are objects. Make it a separate named export instead, which also allows for strictly typing the allowable property names. * Make the `url` field of the `ConfigMenuItem` type optional as this was now recognized as a type error. This matches documentation and actual usage. * Rename the `dagMaxServicesLen` property definition to `dagMaxNumServices` as that is what the actual code uses. * Fix the `linkPatterns` config option typedef to be an array, as per actual usage. * Update `handleError` in Plexus to correctly take either an `ErrorEvent` or a `MessageEvent` to match updated typings for the Web Worker API. * Update `catch` clauses in `readJsonFile` because TypeScript 4.4+ [types the error in `catch` clauses as `unknown` by default](https://devblogs.microsoft.com/typescript/announcing-typescript-4-4/#using-unknown-in-catch-variables). We don't expect anything outside of an `Error` to be thrown here, so we can restore the previous behavior. Signed-off-by: Máté Szabó <mszabo@fandom.com>
Which problem is this PR solving?
Short description of the changes
Update TypeScript, ESLint and
@typescript-eslintto the latest versions.Notable changes:
@typescript-eslintprovides TypeScript-specific equivalents for them, and change existing suppressions referencing the now-disabled core rules. Group these together in the ESLint config for clarity.serviceGraph.tsxandtypes.tsx.defaultConfigand the user-defined config wasn't correctly inferred asConfigbefore the upgrade. Fix various type errors resulting from this:'__mergeFields', i.e. the list of config options where user-specified values should be merged with the default object values indefaultConfig, was defined viadefinePropertyand so seemingly inferred as an unknown type. This now causes a type error when iterating over its keys to try and merge the relevant property values, as TypeScript can't guarantee that the property values are objects. Make it a separate named export instead, which also allows for strictly typing the allowable property names.urlfield of theConfigMenuItemtype optional as this was now recognized as a type error. This matches documentation and actual usage.dagMaxServicesLenproperty definition todagMaxNumServicesas that is what the actual code uses.linkPatternsconfig option typedef to be an array, as per actual usage.handleErrorin Plexus to correctly take either anErrorEventor aMessageEventto match updated typings for the Web Worker API.catchclauses inreadJsonFilebecause TypeScript 4.4+ types the error incatchclauses asunknownby default. We don't expect anything outside of anErrorto be thrown here, so we can restore the previous behavior.