Skip to content
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.

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

Playing with TypeScript #1272

Merged
merged 12 commits into from
Mar 31, 2020
Merged

Conversation

ograu
Copy link
Contributor

@ograu ograu commented Mar 27, 2020

The purpose of this PR and this other PR is to kickstart a discussion to decide whether we finally want to add TypeScript, as this is somthing still to be decided to the mix.

In this PR:

  • I converted some small UI components to .ts, and then
  • Converted NodePool component, which is a connected one
  • Created a types.ts file for reuse interfaces
  • Also there is a minor fixing for CSSBreakpoints importing

After two days hacking with TypeScript, these are my thoughts:

  • It is clear that TS offer some great advantatges, being the most remarkable self-documentation of the code and getting rid of type errors, IMO.
  • I'm not used to typed languages, so using TS feels superweird to me. I think it adds a lot of noise and it makes more difficult to focus on getting things done. I think this is just a phase that I would overcome after some weeks working with TS.

And my opinion about if we should migrate to TS or not:

  • From a purely selfish poin of view, I would say yes. I would like to experiment with this, especially taking into account that usage of TS in frontend frameworks in becaming mainstream.
  • Is this good for Happa? Yes, sure.
  • Is this needed for Happa? No, we can live without this.
  • Would the benefits of using TS outweigh the drawbacks? In the short/medium term, I don't think so. In the long term, I would say yes.

Finally, food for touhgt, you will find controversial and subjective (but totally valid IMO) opinions in this relatively short vid (20 min) from MPJ, they talk also about linters: https://www.youtube.com/watch?v=_ECuXk4MX6Y

This vid pretty much sums up how I feel right now! 😬

Copy link
Contributor

@axbarsan axbarsan left a comment

Choose a reason for hiding this comment

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

Looking really good for now! Just left a couple of pointers

src/shared/types.ts Outdated Show resolved Hide resolved
src/components/UI/ErrorFallback.tsx Outdated Show resolved Hide resolved
src/components/Home/UpgradeNotice.tsx Outdated Show resolved Hide resolved
src/components/Home/ClusterDashboardNodes.tsx Show resolved Hide resolved
src/components/Cluster/ClusterDetail/NodePool.tsx Outdated Show resolved Hide resolved
src/components/Cluster/ClusterDetail/NodePool.tsx Outdated Show resolved Hide resolved
src/components/Cluster/ClusterDetail/NodePool.tsx Outdated Show resolved Hide resolved
src/components/Cluster/ClusterDetail/NodePool.tsx Outdated Show resolved Hide resolved
src/components/Cluster/ClusterDetail/NodePool.tsx Outdated Show resolved Hide resolved
src/components/Cluster/ClusterDetail/NodePool.tsx Outdated Show resolved Hide resolved
@oponder
Copy link
Contributor

oponder commented Mar 27, 2020

I'm not used to typed languages, so using TS feels superweird to me. I think it adds a lot of noise and it makes more difficult to focus on getting things done. I think this is just a phase that I would overcome after some weeks working with TS.

I was in the same boat while learning Go, coming from Ruby and PHP. It felt really weird and kind of annoying that I had to specify everything up front.

But after doing that for a while and then going back to working on Happa in Javascript, I started to miss some of the guarantees and editor features that a typed language gave me.

What I think is also really neat about Typescript is that we can apply the typing where we need it, remaining pragmatic, and getting kind of 'best of both worlds' in some way.

@ograu
Copy link
Contributor Author

ograu commented Mar 27, 2020

I was in the same boat while learning Go, coming from Ruby and PHP. It felt really weird and kind of annoying that I had to specify everything up front.

My emotional side is telling me all the time Typescript sucks!!! 😡, while my head is like Take it easy, it will take some time 🎶 ☯️ 😌

What I think is also really neat about Typescript is that we can apply the typing where we need it, remaining pragmatic, and getting kind of 'best of both worlds' in some way.

Yes, that's definitely supercool, we can define how much TS we want to use. Otherwise migrations would become impossible.

@ograu
Copy link
Contributor Author

ograu commented Mar 27, 2020

Looking really good for now! Just left a couple of pointers

Thanks @axbarsan!
Finally it was more than "a couple"! 😬 🙏

this.setState({ isNameBeingEdited });
};

triggerEditName = () => {
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this isn't needed anymore

const { cluster, nodePool } = this.props;

try {
this.props.dispatch(
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here


function mapStateToProps(state, ownProps) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function mapStateToProps(state: Record<string, any>, ownProps: INodePoolProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function mapStateToProps(state: Record<string, any>, ownProps: INodePoolProps) {
function mapStateToProps(state: Record<string, any>, ownProps: INodePoolProps): IStateProps {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IStateProps have more properties defined...

Does it makes sense to have a type for this if we are defining this prop in the props interface?

Copy link
Contributor

Choose a reason for hiding this comment

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

But IStateProps is supposed to contain only the props that are retrieved from the redux state.
It is useful to have them defined separately for each component (INodePoolProps, IStateProps, IDispatchProps), because it's easier to know while developing where the props are coming from (if it's an own prop, or an action, or a prop coming from the state)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 ok, I get it, thanks

@axbarsan axbarsan merged commit b6782bd into configure-typescript Mar 31, 2020
@axbarsan axbarsan deleted the components-to-typescript branch March 31, 2020 08:01
axbarsan added a commit that referenced this pull request Mar 31, 2020
* Configure typescript

* Resolve linter errors

* Add missing types

* Remove extensions from import statement

* Type check theme and styled components

* Revert to using global styled

* Enable typing for styled theme

* Maybe run yarn install next time

* Configure linter and fix failing code

* Add linter default typescript path

* Convert eslint config to javascript, to be able to properly configure the ts root path

* Adjust the tsconfig path to be at the root of the tsconfig root directory

* Expose tsconfig file to docker

* One more shot

* Convert login to typescript

* Convert VersionPicker to TypeScript

* Update src/components/Auth/Login.tsx

* Playing with TypeScript (#1272)

* Fix linter warnings

* ErrorFallback to ts

* ClusterDashboardNodes to ts

* UpgradeNotice to ts

* NodePool to ts

* index.html

* Functions  types in NodePool

* Added types file

* Fixings

* More fixes

* Make fixes to satisfy typechecker

Co-authored-by: axbarsan <[email protected]>

* Fix typechecker error

* Fix types in VersionPicker

Co-authored-by: ograu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants