Frontend Project Health Improvements

[Bobgy]

UPDATE: this is done, future work tracked in https://github.com/kubeflow/pipelines/projects/4

This is an umbrella issue to track action items to improve frontend code base project health.

Goal is to improve onboarding, developer experience and facilitate contribution.

Planning

• p0 Standardize code style using prettier
• p0 Tslint should ignore generated code
• p0 Upgrade typescript to 3.6
• p0 0.2w Refactor frontend to use promise based grpc-web metadata api
• p0 0.1w Document and fix recommended npm daily practices
• p0 p1 0.8w (because p0 issues already resolved) Set up unit tests for frontend node server
• p0 0.1w Document how to set up frontend dev env using mock server/kubeflow deployment
• p1 0.6w Migrate to create-react-app and typescript-eslint
• p1 0.4w Frontend node server strict type checking
• p2 0.4w Document how to run end-to-end tests locally [Frontend pH] document how to run frontend-integration tests locally #3600
• p1 Typescript lint developer experience improvements
• p1 Upgrade node to 12 LTS
• p1 Enable optional chaining js feature (blocked by migrating to create-react-app)
• p2 [UI] Consider migrating from enzyme to react-testing-library
• p2 Update frontend integration tests to work with latest selenium chrome
• Snapshot diff best practices

[Bobgy]

/area front-end
#frontend-pH

[Bobgy]

Hi @eterna2, do you want to add anything like pain points here?

[eterna2]

Looks great to me. I will add here if I think anything else.

Not sure if we wish to impose consistency in the usage of async/await vs promise chaining because I notice the node backend uses both.

[Bobgy]

@eterna2 Thanks for the suggestion. I've not written much code for node backend. Which do you prefer? I think async/await is clearer and easier to write by all means. We can recommend switching to it doing new changes.

Do you think we need a strict rule (maybe https://github.com/xjamundx/eslint-plugin-promise/blob/master/docs/rules/prefer-await-to-then.md)? We cannot use an eslint rule right now, but after we switch to eslint-typescript. Maybe we can use it.

[eterna2]

I generally prefer async/await over promises too because of the readability.

Would nice to enforce this consistency just to reduce confusion on how we shld handle async functions.

Just need to be careful when using them, as u can incorrectly blocks the thread w/o actually needing to.

Here is a good article on it:
https://hackernoon.com/javascript-async-await-the-good-part-pitfalls-and-how-to-use-9b759ca21cda

[Bobgy]

I generally prefer async/await over promises too because of the readability.

Would nice to enforce this consistency just to reduce confusion on how we shld handle async functions.

@eterna2 I think that's a good idea. Do you want to contribute to this or any of the items above? I don't think I can have time for this refactoring shortly.

Rough ideas:

1. We should first complete [Frontend] Set up unit tests for frontend node server #2460 for test coverage.
2. Convert existing usage of promise chain to async/await.
3. (optional) Add a code style document recommending best practices or enforce by lint rules.
   (Maybe we don't even need this, when our code base only uses async/await, I think contributors will discover that consistency by themselves.)

[jingzhang36]

I ran into an issue where FE complier complains about casing difference. We probably can remove the check at

pipelines/frontend/tsconfig.json

Line 17 in d315bf6

"forceConsistentCasingInFileNames": true,

[Bobgy]

New issue that came out:

• Configure jest to hide noisy message about no coverage data on .d.ts files Configure jest to hide noisy message about no coverage data on .d.ts files #2672

[Bobgy]

Another new issue I found out:

• p1 Inconsistency between building in frontend/server folder locally and in docker.

[Bobgy]

Most important issues have all been fixed!!
The remaining tasks will be tracked in https://github.com/kubeflow/pipelines/projects/4
/close

[k8s-ci-robot]

@Bobgy: Closing this issue.

In response to this:

Most important issues have all been fixed!!
/close

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.