TypeScript Enhancements #1757

atiratree · 2019-06-19T10:05:16Z

No description provided.

openshift-ci-robot · 2019-06-19T10:05:31Z

Hi @suomiy. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

atiratree · 2019-06-19T10:05:40Z

frontend/packages/console-shared/src/selectors/common.ts

in order to have typed selectors

frontend/public/module/k8s/index.ts

atiratree · 2019-06-19T10:07:58Z

frontend/public/module/k8s/index.ts

to conform to the API:
for example please see "io.k8s.api.core.v1.Pod": { in https://raw.githubusercontent.com/openshift/origin/master/api/swagger-spec/openshift-openapi-spec.json

These are optional types mainly because when fetching a list of k8s resources (using k8sList()), the apiVersion and kind are often omitted from the individual list items.

atiratree · 2019-06-19T10:15:31Z

This functionality serves as a base PR for #1731

spadgett · 2019-06-19T14:54:24Z

frontend/packages/console-shared/src/types/index.ts

We already have this defined here:

https://github.com/openshift/console/blob/master/frontend/public/components/utils/index.tsx#L94-L98

Thanks, missed that..

spadgett · 2019-06-19T14:56:42Z

frontend/public/module/k8s/index.ts

Isn't this always a string?

I am playing with type generation currently (please see #1731). The disscussion is still ongoing if to use a separate repo for the types, pulling them to our code base or not having them.

I found out that some types are generated as string (kubevirt types) but some are generated as Date (openshift types; https://github.com/openshift/origin/blob/master/api/swagger-spec/openshift-openapi-spec.json)

So I figured it might not hurt to have the both options here (used by @console/shared selectors)

But I can remove this for now until the issue with types is resolved, if you prefer.

spadgett · 2019-06-19T14:57:41Z

frontend/package.json

This seems useful, although I'm a little hesitant to rely on a project with so little history and only one contributor.

@christianvogt @vojtechszocs opinion?

cc @alecmerdler

I'd have to play around with it to see if it's more useful than _.get(...) as SomeType, which is what we tend to use now.

Yes, that project is quite new but it is also extremely small - just one wrapper function (https://github.com/RIP21/ts-get/blob/master/src/index.ts). So it would be easy to pull it into our codebase if it goes under.

I dislike using _.get because it's easy to miss when refactoring. Sure casting the result satisfies typescript but it's a crutch.

My first hunch regarding ts-get was that it used try-catch which is going to be slow. Sure enough they have a closed issue of performance:
RIP21/ts-get#3

My first hunch regarding ts-get was that it used try-catch which is going to be slow. Sure enough they have a closed issue of performance:
RIP21/ts-get#3

Oh, wow, that is quite a difference

christianvogt · 2019-06-19T16:59:12Z

frontend/packages/console-shared/src/selectors/common.ts

Could just as easily make the code explicit:

Suggested change

export const getUid = (value: K8sResourceKind) => get(value, (v) => v.metadata.uid);

export const getUid = (value: K8sResourceKind) => value.metadata ? value.metadata.uid : undefined;

We'd want to check for value being undefined as well

Sorry I'm use to working in projects with strict tsconfig options enabled: strictNullChecks = true. I saw value and not value? and assumed it cannot be null|undefined. We should look at tightening the tsconfig settings.

Suggested change

export const getUid = (value: K8sResourceKind) => get(value, (v) => v.metadata.uid);

export const getUid = (value: K8sResourceKind) => value && value.metadata && value.metadata.uid || undefined;

atiratree · 2019-06-20T11:07:19Z

frontend/packages/console-shared/src/selectors/common.ts

removed the ts-get dependency due to the performance issues as discussed.

I do not like the null checking along the way as proposed because it is easy to make a mistake and is tedious to write for longer paths. So I resolved it with type casting.

Thoughts?

Both approaches today are error prone. If we turn on strictNullChecks then being explicit is a better approach because we benefit from full typechecks. _.get will always be error prone.

Shouldn't we rather wait for strictNullChecks to be agreed upon and proceed with this? Such a change would certainly affect the whole code base.

I wasn't implying we enable strictNullChecks.

I'm fine with using _.get. It's used everywhere already and it seems others are ok with continuing to use it over a few null checks.

atiratree · 2019-06-25T14:04:40Z

frontend/public/module/k8s/index.ts

I reverted this back to just a string. The type generation will be configurable to map this property to string.

instead of a Date

spadgett · 2019-06-25T15:35:14Z

/assign @alecmerdler

alecmerdler

/lgtm

alecmerdler · 2019-06-25T16:33:59Z

/ok-to-test

atiratree · 2019-06-25T17:25:16Z

frontend/packages/dev-console/src/utils/imagestream-utils.ts

  const builderImageStreams = imageStreams.filter((imageStream) => isBuilder(imageStream));

-  return builderImageStreams.reduce((builderImages, imageStream) => {
+  return builderImageStreams.reduce((builderImages: NormalizedBuilderImages, imageStream) => {


ERROR in /go/src/github.com/openshift/console/frontend/packages/dev-console/src/utils/imagestream-utils.ts (90,3): Type 'K8sResourceKind' is not assignable to type 'NormalizedBuilderImages'. Property 'apiVersion' is incompatible with index signature. Type 'string' is not assignable to type 'BuilderImage'.

this type error appeared after the changes of apiVersion -> apiVersion?. Not sure why this triggered it but the type has to be specified for the accumulator in reduce function.

atiratree · 2019-06-25T20:33:56Z

/test e2e-aws

alecmerdler

/lgtm

openshift-ci-robot · 2019-06-25T21:29:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alecmerdler, suomiy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [alecmerdler]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot requested review from jcaianirh and spadgett June 19, 2019 10:05

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 19, 2019

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 19, 2019

atiratree commented Jun 19, 2019

View reviewed changes

atiratree mentioned this pull request Jun 19, 2019

VirtualMachine details: add Disks tab #1731

Merged

2 tasks

atiratree force-pushed the console.type-fixes branch from 2125f0f to d196a73 Compare June 19, 2019 10:50

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 19, 2019

atiratree force-pushed the console.type-fixes branch from d196a73 to e3c8eca Compare June 19, 2019 10:58

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 19, 2019

atiratree mentioned this pull request Jun 19, 2019

Kubevirt: port VM list to PF4 Table #1758

Merged

2 tasks

spadgett reviewed Jun 19, 2019

View reviewed changes

atiratree force-pushed the console.type-fixes branch from e3c8eca to f93cc7c Compare June 19, 2019 15:46

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 19, 2019

christianvogt reviewed Jun 19, 2019

View reviewed changes

spadgett added this to the v4.2 milestone Jun 19, 2019

atiratree force-pushed the console.type-fixes branch from f93cc7c to c3b10ad Compare June 20, 2019 10:59

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 20, 2019

atiratree force-pushed the console.type-fixes branch from c3b10ad to 17919f4 Compare June 20, 2019 11:01

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 20, 2019

atiratree commented Jun 20, 2019

View reviewed changes

atiratree mentioned this pull request Jun 20, 2019

Introduce TS types for OpenShift / KubeVirt #1773

Closed

1 task

atiratree force-pushed the console.type-fixes branch from 17919f4 to e62a2cb Compare June 25, 2019 14:02

atiratree commented Jun 25, 2019

View reviewed changes

openshift-ci-robot assigned alecmerdler Jun 25, 2019

suomiy added 3 commits June 25, 2019 18:20

K8sResourceKind: conform to API types

32bb85b

introduce types to @console/shared

cdee611

TableData: add possibility to pass id to the table cell

e5e8283

atiratree force-pushed the console.type-fixes branch from e62a2cb to e5e8283 Compare June 25, 2019 16:20

alecmerdler approved these changes Jun 25, 2019

View reviewed changes

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 25, 2019

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 25, 2019

imagestream-utils: fix TS error

5f4d95f

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2019

atiratree commented Jun 25, 2019

View reviewed changes

alecmerdler approved these changes Jun 25, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2019

openshift-merge-robot merged commit 2c9c6ca into openshift:master Jun 25, 2019

	export const getUid = (value: K8sResourceKind) => get(value, (v) => v.metadata.uid);
	export const getUid = (value: K8sResourceKind) => value.metadata ? value.metadata.uid : undefined;

TypeScript Enhancements #1757

TypeScript Enhancements #1757

Uh oh!

Conversation

atiratree commented Jun 19, 2019

Uh oh!

openshift-ci-robot commented Jun 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atiratree commented Jun 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atiratree Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spadgett commented Jun 25, 2019

Uh oh!

alecmerdler left a comment

Choose a reason for hiding this comment

Uh oh!

alecmerdler commented Jun 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atiratree commented Jun 25, 2019

Uh oh!

alecmerdler left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jun 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

atiratree Jun 25, 2019 •

edited

Loading