-
Notifications
You must be signed in to change notification settings - Fork 618
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
CONSOLE-2501: Upgrade TypeScript version to 4.5 #12821
CONSOLE-2501: Upgrade TypeScript version to 4.5 #12821
Conversation
@vojtechszocs: This pull request references CONSOLE-2501 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@vojtechszocs: This pull request references CONSOLE-2501 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some additional comments for reviewers.
@@ -19,13 +20,11 @@ module.exports = { | |||
parser: '@typescript-eslint/parser', | |||
parserOptions: { | |||
project: './tsconfig.json', | |||
comment: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed since this option is not supported.
https://github.com/typescript-eslint/typescript-eslint/tree/v5.43.0/packages/parser
ecmaFeatures: { | ||
jsx: true, | ||
}, | ||
ecmaVersion: 2017, | ||
ecmaVersion: 2018, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All ESLint configs reconciled to use ecmaVersion: 2018
setting.
extraFileExtensions: ['.json'], | ||
sourceType: 'module', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed since this option is not supported.
https://github.com/typescript-eslint/typescript-eslint/tree/v5.43.0/packages/parser
return _.compact( | ||
getJSDoc(node).map((d) => { | ||
if (d.tags) { | ||
return d.tags.map((t) => `@${t.tagName.text} ${t.comment}`).join('\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is due to TypeScript lib API change, where comment
property of all JSDoc[*]
types changed from string
to optional string | NodeArray<JSDocComment>
.
@@ -1,6 +1,6 @@ | |||
import { Extension } from '../types'; | |||
|
|||
export { ResolvedExtension } from '../types'; | |||
export type { ResolvedExtension } from '../types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using export type
fixes an issue in the webpack build, where ts-loader
would assume that e.g. ResolvedExtension
above is exported as a value, while this symbol actually refers to a type.
@@ -1,3 +1,5 @@ | |||
/* eslint-disable @typescript-eslint/naming-convention */ | |||
|
|||
export enum addOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Symbols that violate @typescript-eslint/naming-convention
rule but are exported were not renamed to keep the changes at minimum.
"eslint-plugin-cypress": "^2.10.3", | ||
"eslint-plugin-graphql": "^4.0.0", | ||
"eslint-plugin-import": "^2.22.1", | ||
"eslint-plugin-tsdoc": "^0.2.14", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above ESLint related dependencies were moved to and reconciled with packages/eslint-plugin-console
package manifest.
@@ -2,7 +2,7 @@ import { getCloudInitValues, TARGET_PORT } from '../components/ssh-service/SSHFo | |||
import { VMIKind } from '../types'; | |||
import { useSSHService2 } from './use-ssh-service'; | |||
|
|||
export type useSSHCommandResult = { | |||
type UseSSHCommandResult = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Types not used outside the module whose name violates the ESLint naming rule were renamed and the export
keyword was removed.
@@ -1,5 +1,5 @@ | |||
/** | |||
* @deprecated [TopologySideBar] This files has been moved to @console/topology and delete this once all the side panels uses dynamic extensions | |||
* @deprecated [TopologySideBar] This files has been moved to `@console/topology` and delete this once all the side panels uses dynamic extensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes an ESLint tsdoc/syntax
error where the rule thinks that @console
is an TSDoc tag.
@@ -4,7 +4,7 @@ let lastID = 0; | |||
|
|||
const getNextReduxID = () => String(++lastID); | |||
|
|||
export const withReduxID = <T, _>(Component: React.ComponentType<T>) => { | |||
export const withReduxID = <T,>(Component: React.ComponentType<T>) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using <T,>
is better than <T, _>
since the latter introduces an unused type parameter.
https://dev.to/tlylt/typescript-generic-function-reported-as-jsx-error-57nf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vojtechszocs Nice work! 👍 No major concerns with the change. Let's make sure we have follow-up issues for tech debt we still want to fix (like updating other packages to the latest).
/approve
'{}': false, | ||
Function: false, | ||
object: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might open a follow up tech debt story to remove these types for something better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, there could be a follow-up task to use better type equivalents.
@typescript-eslint/ban-types
rule docs contain suggestions on possible replacement types.
@@ -9,24 +9,26 @@ | |||
"prettier-check": "eslint --print-config ./lib/config/prettier.js | eslint-config-prettier-check" | |||
}, | |||
"dependencies": { | |||
"@typescript-eslint/eslint-plugin": "2.11.0", | |||
"@typescript-eslint/parser": "2.11.0", | |||
"@typescript-eslint/eslint-plugin": "5.43.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine not moving to the absolute latest version in this PR to minimize churn, but let's make sure we open follow-up issues to upgrade.
@@ -1,5 +1,5 @@ | |||
import * as React from 'react'; | |||
// eslint-disable-next-line @typescript-eslint/ban-ts-ignore | |||
// eslint-disable-next-line @typescript-eslint/ban-ts-comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make sure we have an issue tracking upgrading the types if we don't already so we can remove these comments. (Not for this PR, though.)
@@ -1,3 +1,4 @@ | |||
/* eslint-disable @typescript-eslint/no-use-before-define */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we open a follow up issue to fix these? (Or disable this rule globally if we decide it's not a problem?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better code consistency, I think we should address all @typescript-eslint/no-use-before-define
suppressions as a follow-up task, even though technically both TypeScript compiler and webpack can deal with symbols (types or values) used before they are declared.
The fix for that should be low risk, it's just about moving the relevant code bits.
frontend/packages/ceph-storage-plugin/src/components/dashboards/ocs-system-dashboard.tsx
Show resolved
Hide resolved
// eslint-disable-next-line dot-notation | ||
const kmsProvider: ProviderNames = state.kms?.['provider'] || ProviderNames.VAULT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just fix this one?
const kmsProvider: ProviderNames = state.kms?.provider || ProviderNames.VAULT;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes
Property 'provider' does not exist on type 'VaultConfig | KMSConfig'.
which indicates a possible typing issue.
I can fix this by casting to any
-const kmsProvider: ProviderNames = state.kms?.['provider'] || ProviderNames.VAULT;
+const kmsProvider: ProviderNames = (state.kms as any)?.provider || ProviderNames.VAULT;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, let's open a separate bug to investigate and figure out if it's a problem with the types or a real bug.
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
type watchResource = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We might just fix this one if not exported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, will fix.
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
type watchResource = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Fix this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, will fix.
// eslint-disable-next-line react-hooks/rules-of-hooks | ||
const { t } = useTranslation(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's open a bug to fix this as a follow up. I'm surprised this works.
expect(lastFirehoseChildProps.pods.data).not.toBe(lastUseResourcesHookResult.pods.data); // Could be the same! | ||
expect(lastFirehoseChildProps.pods.data[0]).not.toBe(lastUseResourcesHookResult.pods.data[0]); // Should be the same!!! | ||
expect(lastFirehoseChildProps.pods.data[1]).not.toBe(lastUseResourcesHookResult.pods.data[1]); // Should be the same!!! | ||
expect(lastFirehoseChildProps.pods.data[2]).not.toBe(lastUseResourcesHookResult.pods.data[2]); // Should be the same!!! | ||
expect(lastFirehoseChildProps.pods.data).toBe(lastUseResourcesHookResult.pods.data); | ||
expect(lastFirehoseChildProps.pods.data[0]).toBe(lastUseResourcesHookResult.pods.data[0]); | ||
expect(lastFirehoseChildProps.pods.data[1]).toBe(lastUseResourcesHookResult.pods.data[1]); | ||
expect(lastFirehoseChildProps.pods.data[2]).toBe(lastUseResourcesHookResult.pods.data[2]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, we fixed a bug by adding useMemo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what exactly fixed this.. after my changes, the above unit test started failing and then I saw the "Could / Should be the same" comments so I thought it's OK to change these expectations.
/retest |
/test e2e-gcp-console |
lets see if the node16 is in place |
/test e2e-gcp-console |
/test images |
/retest |
@XiyunZhao will verify the changes |
I feel lucky this time 🤞 |
Fingers crossed
/retest |
/test ceph-storage-plugin |
/retest |
2 similar comments
/retest |
/retest |
/test ceph-storage-plugin |
1 similar comment
/test ceph-storage-plugin |
Do we know if the ceph-storage-plugin tests are working? I don't see any history since they only run when the package is changed. @bipuladh Can you help look? |
@spadgett please override the tests. We have not maintained them in a while. I will send a PR to remove these tests. |
/override ci/prow/ceph-storage-plugin |
@jhadvig: Overrode contexts on behalf of jhadvig: ci/prow/ceph-storage-plugin In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
QE Approver: |
/label docs-approved |
/label px-approved |
A regression test has been triggered for this PR, all the failed case has been checked manually, and no issue was found |
@vojtechszocs: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Related to #10432 and #10604
This PR bumps TypeScript (TS) version used by Console to
4.5
along with bumping other TS related dependencies.Note that bumping TS beyond
4.5
would yield even more changes in terms of dependencies, code and tool configuration. Therefore, further TS version bumps should be realized via follow-up PRs.Possible follow-up improvements for improving code quality and dependencies are listed below.
Notable dependency bumps
ts-loader
to 8.4.0fork-ts-checker-webpack-plugin
to 6.5.2typescript
parser could not process newer TS syntax such asexport type
@typescript-eslint/typescript-estree
dependency resolution while keeping Prettier at current version1.19.1
did not solve this problemeslint-plugin-react-hooks
to 4.1.2eslint-plugin-cypress
to 2.11.3packages/kubevirt-plugin
Notable changes
4.5
is used for the whole monorepo (removed TS dependency in@console/dynamic-plugin-sdk
package)jest.spyOn
which does not work well with TS generated property accessors for module exports@console/plugin-sdk/src/api/useExtensions
instead of@console/plugin-sdk
) seems to work around the above mentionedjest.spyOn
limitation@typescript-eslint/camelcase
and@typescript-eslint/class-name-casing
were replaced by a more general rule@typescript-eslint/naming-convention
(see here for the old list and here for the replacement rule docs)@typescript-eslint/ban-ts-ignore
was replaced by@typescript-eslint/ban-ts-comment
eslint-plugin-react-hooks
bumpreact-hooks/exhaustive-deps
now detects more potential issues, code adapted where possible while trying to keep the changes to minimumPossible follow-up improvements
@typescript-eslint/ban-ts-comment
@typescript-eslint/no-use-before-define
@typescript-eslint/naming-convention
tsdoc/syntax
cc @spadgett @jhadvig