ui - enhance flow typing #3002

JBWatenbergScality · 2020-12-24T15:49:31Z

Component:

ui, typing

Context:

This pull request is a first milestone to introduce better typings in the project.
This brings type-safety and enhanced developer experience to the project.

As introducing strong typing everywhere in the project would require a few more pull requests the idea of this one is to focus on kubernetes client and reducers that make use of kubernetes client. However it also brings typings for few others reducers and introduce starting points to expand typing to other areas in the project.

Closes: #2999

See: #3000 #3001

bert-e · 2020-12-24T15:49:33Z

Hello jbwatenbergscality,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

bert-e · 2020-12-24T15:49:36Z

Branches have diverged

This pull request's source branch improvement/flow-typing has diverged from
development/2.7 by more than 50 commits.

To avoid any integration risks, please re-synchronize them using one of the
following solutions:

Merge origin/development/2.7 into improvement/flow-typing
Rebase improvement/flow-typing onto origin/development/2.7

Note: If you choose to rebase, you may have to ask me to rebuild
integration branches using the reset command.

bert-e · 2020-12-24T15:55:03Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

gdemonet

Super cool stuff, thanks for starting this heavy lifting, I'm confident this will increase productivity a lot!
Some comments and suggestions here and there 😉 , just adding a more general one here: are you running Prettier? We have a committed .prettierrc for you to use, the goal being to keep the code consistent without spending time on aesthetics in PR reviews 😜

gdemonet · 2020-12-28T09:07:46Z

ui/src/services/k8s/api.js

+  kind: 'Volume',
+  metadata?: V1ObjectMeta,
+  spec: {
+    mode: 'Filesystem',


The mode can be either Filesystem or Block, actually. See our docs or the CRD

gdemonet · 2020-12-28T09:15:17Z

ui/src/services/k8s/api.js

+    storageClassName: string
+  },
+  status: {
+    deviceName: string,


If this was not yet reconciled (e.g. because the Volume is Pending), this will be empty from the Operator's PoV, and absent in the API object.

gdemonet · 2020-12-28T09:16:05Z

ui/src/services/k8s/api.js

+    },
+    storageClassName: string
+  },
+  status: {


If you want to be exhaustive, there is also a status.job key (which is also optional, and its usage is internal to the Operator).

gdemonet · 2020-12-28T09:21:02Z

ui/src/ducks/app/volumes.js

  return { type: UPDATE_VOLUMES, payload };
 };

-export const fetchCurrentVolumeObjectAction = (volumeName) => {
+export const fetchCurrentVolumeObjectAction = (volumeName: any) => {


I guess we can safely assume this should be a string, no?

gdemonet · 2020-12-28T10:16:58Z

ui/src/ducks/app/volumes.js

+export function* fetchVolumes(): Generator<{
+  body: {items: Metalk8sV1Alpha1Volume[]};
+} | {error: any, body: null}, {
+  body: {items: Metalk8sV1Alpha1Volume[]};
+} | {error: any, body: null}, {
+  body: {items: Metalk8sV1Alpha1Volume[]};
+} | {error: any, body: null}> {


I'm not sure that's a valid type for sagas. IIUC, the type signature for a coroutine is Generator<Yield, Return, Next>. Sagas only yield "effects" (in the sense of redux-saga), and they may be fed with anything.
For this reason, there is a handy type definition in flow-typed for the redux-saga library:

import type { Saga } from 'redux-saga'; // same as: declare type Saga<T> = Generator<Effect, T, any>

See the definitions here

Thanks I was looking for those typings desperately 👍 👍

gdemonet · 2020-12-28T13:31:16Z

ui/src/ducks/config.js

+export type ConfigState = {
+  language: string,
+  theme: Theme,
+  api: Config | null,


?Config would work here as well, IIUC

gdemonet · 2020-12-28T13:33:21Z

ui/src/ducks/config.js

  return { type: SET_THEMES, payload: themes };
 }

 // Selectors
-export const languageSelector = (state) => state.config.language;
-export const apiConfigSelector = (state) => state.config.api;
+export const languageSelector = (state: RootState) => state.config.language;


Just a question: since RootState is properly typed, is flow able to infer the return type of these selectors?

Yes, flow is inferring the type of languageSelector from state.config.language

gdemonet · 2020-12-28T13:37:19Z

ui/src/ducks/login.js

-  const result = yield call(ApiSalt.authenticate, user);
-  if (!result.error) {
+export function* authenticateSaltApi(): Generator<any, void, any> {
+  const api: Config | null = yield select(apiConfigSelector);


(ugh, even if flow was able to infer the return type of a selector, it can't guess the value injected by the coroutine's "owner"... some big limitation of sagas here)

Yes the simplest way I found to type complex saga with different return type over iterations is to type Next as any and force the type of each of the variables 😞 . Even through this is not directly linked to redux but more to typing, IMO redux and redux saga are adding a lot of complexity for such simple tasks as data fetching, I prefer using libraries such as react-query or swr that greatly ease data fetching and caching use cases and that are way more easier to type as in most cases the typing would be inferred from API clients typing.

gdemonet · 2020-12-28T13:46:25Z

ui/src/ducks/login.js

+export function* authenticateSaltApi(): Generator<any, void, any> {
+  const api: Config | null = yield select(apiConfigSelector);
+  const user: User = yield select((state: RootState) => state.oidc?.user);
+  const result: { error: any } | ApiSalt.SaltToken = yield call(ApiSalt.authenticate, user);


Could be handy to have type aliases for such "result" values:

declare type Result<T> = { error: any } | T // Or for API results under a `body` key declare type APIResult<T> = Result<{ body: T }>

Here we'd have:

const result: Result<Api.SaltToken> = yield call(...)

gdemonet · 2020-12-28T13:54:02Z

ui/src/ducks/reducer.js

+export const useTypedSelector: <TSelected>(
+  selector: (state: RootState) => TSelected,
+  equalityFn?: (left: TSelected, right: TSelected) => boolean
+) => TSelected = useSelector;


Two comments:

I think we have defined custom hooks in src/services/utils.js(though granted, it doesn't make much sense either) - IMHO, adding a plain src/hooks.js could make it easier to find in the future

the purpose of this custom hook is not obvious at first glance, so I'd suggest we add a comment to explain it (and remind the reader that they should use it instead of the standard useSelector hook)

bert-e · 2020-12-28T13:57:31Z

Conflict

There is a conflict between your branch improvement/flow-typing and the
destination branch development/2.7.

Please resolve the conflict on the feature branch (improvement/flow-typing).

 $ git fetch
 $ git checkout origin/improvement/flow-typing
 $ git merge origin/development/2.7
 $ # <intense conflict resolution>
 $ git commit
 $ git push origin HEAD:improvement/flow-typing

bert-e · 2020-12-29T14:05:25Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

The following reviewers are expecting changes from the author, or must review again:

@gdemonet

gdemonet · 2020-12-29T14:55:58Z

ui-metalk8s-client-generator/README.md

@@ -0,0 +1,23 @@
+## Metalk8s client generator
+
+It generates a flow client based on `@kubernetes/client` custom objects client from kubernetes `CustomResourceDefinition` of metalk8s volumes.


Awesome 👍

ChengYanJin

LGTM!
Can't wait to get it to merge and benefit from the typing.

bert-e · 2021-01-08T08:36:02Z

Conflict

There is a conflict between your branch improvement/flow-typing and the
destination branch development/2.7.

Please resolve the conflict on the feature branch (improvement/flow-typing).

 $ git fetch
 $ git checkout origin/improvement/flow-typing
 $ git merge origin/development/2.7
 $ # <intense conflict resolution>
 $ git commit
 $ git push origin HEAD:improvement/flow-typing

bert-e · 2021-01-08T08:50:10Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/flow-typing into
development/2.7
w/2.8/improvement/flow-typing will be merged into development/2.8

The following branches will NOT be impacted:

development/1.0
development/1.1
development/1.2
development/1.3
development/2.0
development/2.1
development/2.2
development/2.3
development/2.4
development/2.5
development/2.6

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

bert-e · 2021-01-08T08:50:11Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

The following reviewers are expecting changes from the author, or must review again:

@gdemonet

gdemonet

Could we squash the commits into logical units, now that reviewers approved? I'm not too fond of fix stuff commits, especially for fixing things introduced in the same PR.

bert-e · 2021-01-08T17:57:22Z

History mismatch

Merge commit #6e03a09581f9c41616f1bf5816c34e72ea53663c on the integration branch
w/2.8/improvement/flow-typing is merging a branch which is neither the current
branch improvement/flow-typing nor the development branch
development/2.8.

It is likely due to a rebase of the branch improvement/flow-typing and the
merge is not possible until all related w/* branches are deleted or updated.

Please use the reset command to have me reinitialize these branches.

gdemonet · 2021-01-08T18:10:10Z

ui-metalk8s-client-generator/src/generateClient.ts

+  export async function get${singleName}(${singleName}Name: string): Promise<Result<${singleName}>> {
+    if (!customObjects) {
+      return { error: 'customObject has not yet been initialized' };
+    }
+    try {
+      return await customObjects.getClusterCustomObject(
+        '${crdSpec.spec.group}',
+        '${crdSpec.spec.version}',
+        '${crdSpec.spec.names.plural}',
+        ${singleName}Name,
+      );
+    } catch (error) {
+      return { error };
+    }
+  }


Maybe this part should be squashed with ui: add a flow typed client generator (it's more than just ui: use generated metalk8s client and types)

Checked the CI, it looks like unit tests are failing (apparently src/ducks/app/volumes.test.js was not updated to use the generated client, and something changed in the pods state, which makes src/ducks/app/pods.test.js fail).

JBWatenbergScality · 2021-01-13T15:12:03Z

/approve

bert-e · 2021-01-13T15:12:09Z

History mismatch

Merge commit #6e03a09581f9c41616f1bf5816c34e72ea53663c on the integration branch
w/2.8/improvement/flow-typing is merging a branch which is neither the current
branch improvement/flow-typing nor the development branch
development/2.8.

It is likely due to a rebase of the branch improvement/flow-typing and the
merge is not possible until all related w/* branches are deleted or updated.

Please use the reset command to have me reinitialize these branches.

The following options are set: approve

JBWatenbergScality · 2021-01-13T15:13:13Z

/reset

bert-e · 2021-01-13T15:13:23Z

Reset complete

I have successfully deleted this pull request's integration branches.

The following options are set: approve

bert-e · 2021-01-13T15:13:32Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/flow-typing into
development/2.7
w/2.8/improvement/flow-typing will be merged into development/2.8

The following branches will NOT be impacted:

development/1.0
development/1.1
development/1.2
development/1.3
development/2.0
development/2.1
development/2.2
development/2.3
development/2.4
development/2.5
development/2.6

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

The following options are set: approve

Refs: #3000

Refs: #2999

Refs: #3000

Refs: #2999

…edux store data

The goal of this commit is to introduce a generator of a client based upon `@kubernetes/client` custom objects client from kubernetes `CustomResourceDefinition`. The idea behind this is to have always up to date types definition for our custom resource definition and be in sync with the json schema contained within those definitions. Refs: #2999

Refs: #2999

bert-e · 2021-01-18T11:04:50Z

History mismatch

Merge commit #9a1bb97024ac924946876225330345dcd94bf116 on the integration branch
w/2.8/improvement/flow-typing is merging a branch which is neither the current
branch improvement/flow-typing nor the development branch
development/2.8.

It is likely due to a rebase of the branch improvement/flow-typing and the
merge is not possible until all related w/* branches are deleted or updated.

Please use the reset command to have me reinitialize these branches.

The following options are set: approve

JBWatenbergScality · 2021-01-18T11:05:30Z

/reset

bert-e · 2021-01-18T11:05:39Z

Reset complete

I have successfully deleted this pull request's integration branches.

The following options are set: approve

bert-e · 2021-01-18T11:05:47Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/flow-typing into
development/2.7
w/2.8/improvement/flow-typing will be merged into development/2.8

The following branches will NOT be impacted:

development/1.0
development/1.1
development/1.2
development/1.3
development/2.0
development/2.1
development/2.2
development/2.3
development/2.4
development/2.5
development/2.6

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

The following options are set: approve

bert-e · 2021-01-18T11:05:49Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

The following options are set: approve

bert-e · 2021-01-18T17:17:04Z

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

✔️ development/2.8

The following branches will NOT be impacted:

development/1.0
development/1.1
development/1.2
development/1.3
development/2.0
development/2.1
development/2.2
development/2.3
development/2.4
development/2.5
development/2.6
development/2.7

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

Any commit you add on the source branch will trigger a new cycle after the
current queue is merged.
Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

bert-e · 2021-01-18T18:52:43Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/2.8

The following branches have NOT changed:

development/1.0
development/1.1
development/1.2
development/1.3
development/2.0
development/2.1
development/2.2
development/2.3
development/2.4
development/2.5
development/2.6
development/2.7

Please check the status of the associated issue None.

Goodbye jbwatenbergscality.

Refs: #3002

JBWatenbergScality requested a review from a team as a code owner December 24, 2020 15:49

JBWatenbergScality force-pushed the improvement/flow-typing branch from 67edd45 to 208308d Compare December 24, 2020 15:54

gdemonet suggested changes Dec 28, 2020

View reviewed changes

JBWatenbergScality force-pushed the improvement/flow-typing branch from 735be21 to 6d11214 Compare December 29, 2020 14:05

gdemonet reviewed Dec 29, 2020

View reviewed changes

ChengYanJin previously approved these changes Jan 8, 2021

View reviewed changes

JBWatenbergScality dismissed ChengYanJin’s stale review via 932ee75 January 8, 2021 08:50

JBWatenbergScality force-pushed the improvement/flow-typing branch from e7b7008 to 932ee75 Compare January 8, 2021 08:50

gdemonet suggested changes Jan 8, 2021

View reviewed changes

JBWatenbergScality force-pushed the improvement/flow-typing branch from 6f43719 to defb178 Compare January 8, 2021 17:57

gdemonet reviewed Jan 8, 2021

View reviewed changes

JBWatenbergScality force-pushed the improvement/flow-typing branch from defb178 to 71db196 Compare January 8, 2021 18:38

gdemonet previously approved these changes Jan 9, 2021

View reviewed changes

JBWatenbergScality force-pushed the improvement/flow-typing branch from 71db196 to ffbed03 Compare January 11, 2021 09:49

ChengYanJin previously approved these changes Jan 13, 2021

View reviewed changes

JBWatenbergScality added 14 commits January 18, 2021 12:04

ui: Start node typing

c269126

Refs: #3000

ui: Add notification typings

f46450a

ui: Add pods reducer typing

1420c1f

Refs: #2999

ui: Start salt reducer typing

4f312b3

Refs: #3000

ui: Start solutions reducer typing

056857a

Refs: #2999

ui: Add prometheus alert type

07e99ff

ui: Add root state typing

1022311

ui: Introduce useTypedSelector to enable simpler typed selection of r…

e2509d7

…edux store data

ui: Introduce useTypedSelector in volume page

48e97b3

ui: Use redux-saga Effect in saga typings

29428b9

ui: Use generated metalk8s client and types

0ed1ba9

Refs: #2999

ui: Prettier all the things

28b33c3

ui: Add Namespaced scope support to meatlk8s client generator

dbc6f8b

JBWatenbergScality dismissed ChengYanJin’s stale review via dbc6f8b January 18, 2021 11:04

JBWatenbergScality force-pushed the improvement/flow-typing branch from 7083006 to dbc6f8b Compare January 18, 2021 11:04

ChengYanJin changed the base branch from development/2.7 to development/2.8 January 18, 2021 17:16

ChengYanJin approved these changes Jan 18, 2021

View reviewed changes

bert-e merged commit 68390f4 into development/2.8 Jan 18, 2021

bert-e deleted the improvement/flow-typing branch January 18, 2021 18:52

ChengYanJin added a commit that referenced this pull request Feb 5, 2021

ui/nodealertpage: Address the responsiveness

5f95d92

Refs: #3002

ChengYanJin added a commit that referenced this pull request Feb 5, 2021

ui/volumelist: Address the small screen

d34ade6

Refs: #3002

ChengYanJin added a commit that referenced this pull request Feb 5, 2021

ui/styles: Adapt the common style

0a7838b

Refs: #3002

		@@ -0,0 +1,23 @@
		## Metalk8s client generator

		It generates a flow client based on `@kubernetes/client` custom objects client from kubernetes `CustomResourceDefinition` of metalk8s volumes.

ui - enhance flow typing #3002

ui - enhance flow typing #3002

Conversation

JBWatenbergScality commented Dec 24, 2020

bert-e commented Dec 24, 2020

Hello jbwatenbergscality,

bert-e commented Dec 24, 2020

Branches have diverged

bert-e commented Dec 24, 2020

Waiting for approval

gdemonet 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

JBWatenbergScality Dec 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bert-e commented Dec 28, 2020

Conflict

bert-e commented Dec 29, 2020

Waiting for approval

Choose a reason for hiding this comment

ChengYanJin left a comment

Choose a reason for hiding this comment

bert-e commented Jan 8, 2021

Conflict

bert-e commented Jan 8, 2021

Integration data created

bert-e commented Jan 8, 2021

Waiting for approval

gdemonet left a comment

Choose a reason for hiding this comment

bert-e commented Jan 8, 2021

History mismatch

Choose a reason for hiding this comment

JBWatenbergScality commented Jan 13, 2021

bert-e commented Jan 13, 2021

History mismatch

JBWatenbergScality commented Jan 13, 2021

bert-e commented Jan 13, 2021

Reset complete

bert-e commented Jan 13, 2021

Integration data created

bert-e commented Jan 18, 2021

History mismatch

JBWatenbergScality commented Jan 18, 2021

bert-e commented Jan 18, 2021

Reset complete

bert-e commented Jan 18, 2021

Integration data created

bert-e commented Jan 18, 2021

Waiting for approval

bert-e commented Jan 18, 2021

In the queue

bert-e commented Jan 18, 2021

JBWatenbergScality Dec 28, 2020 •

edited

Loading