Package update #216

gavin-stackrox · 2020-10-23T21:48:56Z

Brings packages up to date using old skool ncu -u.

Testing performed: login, logout, built a GKE cluster with non-standard params, changed lifespan, grabbed artifacts, deleted.

vjwilson

I don't know enough about TS, and esp. TSv4 to say whether all this is the best way. It does worry me that we have to add so many eslint exceptions--but I know some eslint maintainers are "true believers" (in the Eric Hoffer sense).

Perhaps Ivan can shed more light on it.

Other than that, I'm good with this.

vjwilson · 2020-10-23T22:04:57Z

ui/src/client/useApiQuery.ts

+ // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
 setRequestState({ called: true, loading: false, error, data: undefined });


Assume this is needed because of the TypeScript upgrade, but it's not obvious to me why.
The apollo-client React implementation, on which we based this, but for our REST calls, allows undefined.
https://www.apollographql.com/docs/react/api/react/hooks/#result-1

Or is it b/c of something else?

This was due to:
Unsafe assignment of an any value @typescript-eslint/no-unsafe-assignment
I think the 'any' was from error but I really could not figure it out.

Going to review the whole PR, but here looks like TS failed to do the type inference properly, so instead of disabling rules we should just help it by replacing current catch statement with

.catch((error: AxiosError<T>) => {

explicitly specifying the type.

Oh, it's not that it fails to infer, it's just Promise type defined, surprisingly, w/o the explicit type for the error. Anyway, we just can be explicit about the type of error we expect to catch.

vjwilson · 2020-10-23T22:11:06Z

ui/src/components/ErrorBoundary.tsx

@@ -38,17 +38,20 @@ class ErrorBoundary extends Component<PropsWithLocation, State> {
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
 componentDidCatch(error: any): void {
 const { location } = this.props;
+ // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment


Now I'm really worried. Either we need to update our signatures, or this is one of those cases where the eslint folks are so intent on "doing the right thing", they're just dissuading people from using TS.

I think TS is saying just don't use any anywhere time or place. And we have a few uses that would probably need some type acrobatics to refactor.

ikornienko

I agree with Van, too many ESLint exceptions. While they're opinionated, I always follow the rule

Those people are smarted than me;
It's unlikely the code we write is such a super-special snowflake that its above all those rules.

And indeed many of those ignores are unnecessary, as we can just be more specific with types, also making code more strictly typed (yay, more types!).

ikornienko · 2020-10-24T16:13:35Z

ui/src/client/useApiOperation.ts

@@ -37,6 +37,7 @@ export default function useApiOperation<T>(requester: ApiCaller<T>): [() => void
 })
 .catch((error) => {


Same here, just specify the error type with catch((error: AxiosError<T>) => {

ikornienko · 2020-10-24T16:20:56Z

ui/src/components/ErrorBoundary.tsx

@@ -38,17 +38,20 @@ class ErrorBoundary extends Component<PropsWithLocation, State> {
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
 componentDidCatch(error: any): void {


Gavin, can you please

change definition of type State = to have prop error: Error | null and kill //eslint-disable there;

change this error: any to error: Error;

then remove both //eslint-disable above this line and the one you added in this method.

It's my mistake that I've put any here when it should be either unknown or just more specific Error.

ikornienko · 2020-10-24T16:30:35Z

ui/src/components/ErrorBoundary.tsx

 this.setState({ error, errorLocation: location });
 }

 render(): ReactElement {
 const { message, children } = this.props;
+ // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment


That can be removed once you change State type to have prop error typed as Error and not any.

ikornienko · 2020-10-24T16:30:52Z

ui/src/components/ErrorBoundary.tsx

 const { error } = this.state;

 if (error) {
 return (
 <div className="flex h-full items-center justify-center text-base-600">
 <XSquare size="48" />
+ {/* eslint-disable-next-line @typescript-eslint/no-unsafe-member-access */}


And this can be just removed as well.

ikornienko · 2020-10-24T16:36:01Z

ui/src/components/StackRoxLogo.tsx

@@ -1,6 +1,6 @@
 import React, { forwardRef } from 'react';

-const StackRoxLogo = forwardRef<SVGSVGElement, {}>((_, ref) => (
+const StackRoxLogo = forwardRef<SVGSVGElement, unknown>((_, ref) => (


unknown isn't correct. Quite the opposite, we know that the component doesn't take any props, so there are two options as I see it:

use never instead of unknown (we never expect props to be passed)

just remove it, i.e. make it forwardRef<SVGSVGElement>, second type parameter will become the default value as defined in function forwardRef<T, P = {}> (i.e. {} again :) ).

ikornienko · 2020-10-24T19:48:08Z

ui/src/containers/LaunchClusterPage/ClusterForm.tsx

 });
 const initialValues: FormikValues = {
 ID: flavorId,
 Description: '',
 Parameters: createInitialParameterValues(flavorParameters),
 };

- // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-assignment
 const [error, setError] = useState<any>();


Hm, if I understand the code correctly, here we should be doing useState<AxiosError<V1ResourceByID>>();, then you can remove all // eslint-disable above.

ikornienko · 2020-10-24T19:56:47Z

ui/src/containers/LaunchClusterPage/ClusterForm.tsx

@@ -249,7 +249,9 @@ export default function ClusterForm({
 <Form className="md:w-1/3">
 {error && (
 <div className="p-2 mb-2 bg-alert-200">
+ {/* eslint-disable-next-line @typescript-eslint/no-unsafe-member-access */}


You can remove this line once you updated useState above.

ikornienko · 2020-10-24T19:56:52Z

ui/src/containers/LaunchClusterPage/ClusterForm.tsx

 {`[Server Error] ${error.message || 'Cluster creation request failed'}`}
+ {/* eslint-disable-next-line @typescript-eslint/no-unsafe-member-access */}


Hm... ok, that's tricky, V1ResourceByID that I suggested above doesn't have error, but none of the data types in api.ts do... Was I delusional when I wrote this line, as no way the backend can return error with the response data? Or it can it's just swagger / proto definitions are off?

Assuming the backend can return error this way, there are two options:

Fix swagger definitions and regenerate client, using the right type in useState<> above (instead of V1ResourceByID). Yet haven't dug deep enough to understand if it's actually a limit of client generator.. but somewhere there should be a type that defines that backend can return error in response data, shouldn't it?

Hack-y way... change useState above to something like

// actually backend can return `error` with as a prop on `data`, hack to indicate it, as there are limiting factors to define it properly with proto / swagger const [error, setError] = useState<AxiosError<V1ResourceByID & { error: string }>>();

... Was I delusional when I wrote this line, as no way the backend can return error with the response data? Or it can it's just swagger / proto definitions are off?

Assuming the backend can return error this way, there are two options:

Fix swagger definitions and regenerate client, using the right type in useState<> above (instead of V1ResourceByID). Yet haven't dug deep enough to understand if it's actually a limit of client generator.. but somewhere there should be a type that defines that backend can return error in response data, shouldn't it?

Not sure if this is relevant, but I found this article about annotating errors for Go and Swagger,
https://www.ribice.ba/swagger-golang/

ikornienko · 2020-10-24T19:59:38Z

ui/src/setupProxy.js

@@ -1,4 +1,6 @@
 /* eslint @typescript-eslint/no-var-requires: 0, @typescript-eslint/explicit-function-return-type: 0 */
+/* eslint @typescript-eslint/no-unsafe-member-access: 0 */


[Non-action] Hm, some downsides, apparently, of applying @typescript-eslint to .js files... At least we have very few JS files here.

ikornienko · 2020-10-24T21:29:07Z

ui/tsconfig.json

@@ -8,12 +8,14 @@
 "esModuleInterop": true,
 "allowSyntheticDefaultImports": true,
 "strict": true,
+ "strictNullChecks": false, // allow delete in gen-srcs (delete localVarUrlObj.search)


I'm really against this change... It's like we have TS, but we're turning it into JS for the sake of some generated code, ultimately rejecting a benefit of compiler telling us problems before we see in runtime "property blah doesn't exist on type null" errors.

Yet.. I don't know how to disable type checking of sub-directory (i.e. src/generated)... Maybe I'm missing something.

I do have a solution though... switch openapi-generator to 5.0.0-beta2. I.e. in generate-client.sh instead of going to 4.3.1 do

OPENAPI_GENERATOR_CLI_IMAGE_TAG="v5.0.0-beta2"

There they fixed this particular issue with OpenAPITools/openapi-generator#6960. Arguments in favor of using beta2:

it looks like this beta2 is the last one before the final 5.0.0 release

we can always compare the diff of generate code and see if something suspicious is there (I haven't found anything, it's mostly the same except for this URL stuff change)

infra is an internal tool

not that we're shying away from beta versions even in our main product

All of it tells me that we shouldn't do strictNullChecks: false and instead just upgrade the generator.

...
Yet.. I don't know how to disable type checking of sub-directory (i.e. src/generated)... Maybe I'm missing something.
...

We could try adding a subdirectory of a directory being checked to the exclude list in tsconfig.json

Example: https://www.typescriptlang.org/docs/handbook/tsconfig-json.html#examples

AFAIK, it doesn't work this way, see here https://www.typescriptlang.org/tsconfig#exclude Because the files from the generated dir are included through import statement, they're part of the code and therefore will be type checked, adding them to exclude in the tsconfig will do essentially nothing.

gavin-stackrox · 2020-10-26T16:07:11Z

In hindsight, trying to bump up all these packages in a single PR was probably a bad idea. I will close this and try a more incremental and careful upgrade if n' when these upgrades are needed.

vjwilson · 2020-10-26T17:15:40Z

In hindsight, trying to bump up all these packages in a single PR was probably a bad idea. I will close this and try a more incremental and careful upgrade if n' when these upgrades are needed.

That was also what I decided about doing it for the main app, too. It's partly what drove adopting dependabot--to make regular, incremental progress.

ikornienko · 2020-10-26T18:13:38Z

In hindsight, trying to bump up all these packages in a single PR was probably a bad idea. I will close this and try a more incremental and careful upgrade if n' when these upgrades are needed.

I don't know... I felt you were so close :) Just couple of code changes and... Yet you're the boss :)

gavin-stackrox · 2020-10-26T18:22:25Z

Yeah I think it would be quite a time consuming deep dive for me into TS to figure this out. But I'll have this PR handy if'n I (or someone) tries again.

…

On Mon, Oct 26, 2020 at 11:13 AM Ivan Kornienko ***@***.***> wrote: In hindsight, trying to bump up all these packages in a single PR was probably a bad idea. I will close this and try a more incremental and careful upgrade if n' when these upgrades are needed. I don't know... I felt you were so close :) Just couple of code changes and... Yet you're the boss :) — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#216 (comment)>, or unsubscribe <https:/notifications/unsubscribe-auth/APMRL7MJLBTY7YV3DXAY573SMW36FANCNFSM4S5CKC3Q> .

gavin-stackrox added 2 commits October 23, 2020 12:09

WIP: package update

65f4d9a

bump versions

4802c13

gavin-stackrox requested review from vjwilson and pedrottimark October 23, 2020 21:49

vjwilson reviewed Oct 23, 2020

View reviewed changes

gavin-stackrox requested a review from ikornienko October 23, 2020 22:46

ikornienko suggested changes Oct 24, 2020

View reviewed changes

gavin-stackrox closed this Oct 26, 2020

This was referenced Dec 4, 2020

Upgrade openapi-generator to 5.0.0-beta3 #259

Merged

Upgrade react-scripts to 4.0.0 #269

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Package update #216

Package update #216

gavin-stackrox commented Oct 23, 2020

vjwilson left a comment

vjwilson Oct 23, 2020

gavin-stackrox Oct 23, 2020

ikornienko Oct 24, 2020

ikornienko Oct 24, 2020

vjwilson Oct 23, 2020

gavin-stackrox Oct 23, 2020

ikornienko left a comment

ikornienko Oct 24, 2020

ikornienko Oct 24, 2020

ikornienko Oct 24, 2020

ikornienko Oct 24, 2020

ikornienko Oct 24, 2020

ikornienko Oct 24, 2020

ikornienko Oct 24, 2020

ikornienko Oct 24, 2020

vjwilson Oct 25, 2020

ikornienko Oct 24, 2020

ikornienko Oct 24, 2020

vjwilson Oct 25, 2020

ikornienko Oct 25, 2020

gavin-stackrox commented Oct 26, 2020

vjwilson commented Oct 26, 2020

ikornienko commented Oct 26, 2020

gavin-stackrox commented Oct 26, 2020 via email

		// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
		setRequestState({ called: true, loading: false, error, data: undefined });

		@@ -37,6 +37,7 @@ export default function useApiOperation<T>(requester: ApiCaller<T>): [() => void
		})
		.catch((error) => {

		@@ -38,17 +38,20 @@ class ErrorBoundary extends Component<PropsWithLocation, State> {
		// eslint-disable-next-line @typescript-eslint/no-explicit-any
		componentDidCatch(error: any): void {

		{`[Server Error] ${error.message \|\| 'Cluster creation request failed'}`}
		{/* eslint-disable-next-line @typescript-eslint/no-unsafe-member-access */}

		@@ -1,4 +1,6 @@
		/* eslint @typescript-eslint/no-var-requires: 0, @typescript-eslint/explicit-function-return-type: 0 */
		/* eslint @typescript-eslint/no-unsafe-member-access: 0 */

Package update #216

Package update #216

Conversation

gavin-stackrox commented Oct 23, 2020

vjwilson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikornienko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavin-stackrox commented Oct 26, 2020

vjwilson commented Oct 26, 2020

ikornienko commented Oct 26, 2020

gavin-stackrox commented Oct 26, 2020 via email