Skip to content

TypeScript Enhancements #1757

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 4 commits into from
Jun 25, 2019
Merged

TypeScript Enhancements #1757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
32bb85b
K8sResourceKind: conform to API types
suomiy Jun 19, 2019
cdee611
introduce types to @console/shared
suomiy Jun 19, 2019
e5e8283
TableData: add possibility to pass id to the table cell
suomiy Jun 19, 2019
5f4d95f
imagestream-utils: fix TS error
suomiy Jun 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions frontend/packages/console-shared/src/selectors/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,11 @@ import * as _ from 'lodash';

import { K8sResourceKind } from '@console/internal/module/k8s';

export const getName = (value: K8sResourceKind): string => _.get(value, 'metadata.name');
export const getNamespace = (value: K8sResourceKind): string => _.get(value, 'metadata.namespace');
export const getName = (value: K8sResourceKind) =>
_.get(value, 'metadata.name') as K8sResourceKind['metadata']['name'];
export const getNamespace = (value: K8sResourceKind) =>
_.get(value, 'metadata.namespace') as K8sResourceKind['metadata']['namespace'];
export const getUid = (value: K8sResourceKind) =>
_.get(value, 'metadata.uid') as K8sResourceKind['metadata']['uid'];
export const getDeletetionTimestamp = (value: K8sResourceKind) =>
_.get(value, 'metadata.deletionTimestamp') as K8sResourceKind['metadata']['deletionTimestamp'];
Copy link
Member Author

@atiratree atiratree Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@christianvogt christianvogt Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@atiratree atiratree Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@christianvogt christianvogt Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@atiratree atiratree Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

2 changes: 1 addition & 1 deletion frontend/packages/dev-console/src/utils/imagestream-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export const normalizeBuilderImages = (
): NormalizedBuilderImages => {
const builderImageStreams = imageStreams.filter((imageStream) => isBuilder(imageStream));

return builderImageStreams.reduce((builderImages, imageStream) => {
return builderImageStreams.reduce((builderImages: NormalizedBuilderImages, imageStream) => {
Copy link
Member Author

@atiratree atiratree Jun 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 


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.

const tags = getBuilderTagsSortedByVersion(imageStream);
const recentTag = getMostRecentBuilderTag(imageStream);
const { name } = imageStream.metadata;
Expand Down
1 change: 1 addition & 0 deletions frontend/public/components/factory/table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ export const TableData: React.SFC<TableDataProps> = ({className, ...props}) => {
};
TableData.displayName = 'TableData';
export type TableDataProps = {
id?: string;
className?: string;
}

Expand Down
6 changes: 4 additions & 2 deletions frontend/public/module/k8s/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,12 @@ export type ObjectReference = {
export type ObjectMetadata = {
name?: string;
generateName?: string;
uid?: string;
annotations?: {[key: string]: string},
namespace?: string,
labels?: {[key: string]: string},
ownerReferences?: OwnerReference[],
deletionTimestamp?: string;
Copy link
Member Author

@atiratree atiratree Jun 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@atiratree atiratree Jun 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of a Date

[key: string]: any,
};

Expand Down Expand Up @@ -81,8 +83,8 @@ export type Toleration = {
};

export type K8sResourceKind = {
apiVersion: string;
kind: string;
apiVersion?: string;
Copy link
Member Author

@atiratree atiratree Jun 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@alecmerdler alecmerdler Jun 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

kind?: string;
metadata?: ObjectMetadata;
spec?: {
selector?: Selector;
Expand Down