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

[Frontend] typescript lint developer experience improvements #2429

Closed
Bobgy opened this issue Oct 18, 2019 · 5 comments
Closed

[Frontend] typescript lint developer experience improvements #2429

Bobgy opened this issue Oct 18, 2019 · 5 comments
Assignees

Comments

@Bobgy
Copy link
Contributor

Bobgy commented Oct 18, 2019

Pain point: When experimenting with new and dirty code, lint errors and compile errors always stop you asking for a fix, even though the code is completely valid for running (but not clean).

Especially the following issues

Overall, what I consider the ideal developer experience:

  • only show lint errors as warnings during development
  • show also ts comment that disables a certain lint error during development (it's inconvenient to figure out the exact lint name with current set up)
  • fail the CI build if any warning is present

When trying to make the config, I noticed even further that tslint will be deprecated: palantir/tslint#4100 (comment) and it is recommended to migrate to typescript-eslint: https://github.com/typescript-eslint/typescript-eslint#what-about-tslint

Because we are using: https://github.com/wmonk/create-react-app-typescript (which is also deprecated), it doesn't work with eslint. We need to first migrate back to create-react-app which supports typescript natively now.

/area front-end

@Bobgy
Copy link
Contributor Author

Bobgy commented Oct 18, 2019

Temporary workaround that fixes most problems:

  • Use deprecated no-unused-variable rule from tslint
  • Remove noUnusedLocals config from tsconfig.json, but add it in tsconfig.prod.json

In this way:

  • for dev, we get warnings instead of build failures for unused variables
  • for prod, we get errors during CI build

@Bobgy
Copy link
Contributor Author

Bobgy commented Oct 18, 2019

Migrating to create-react-app and typescript-eslint is something we may eventually need to do, but we can wait for stronger reasons for now.
Also, typescript-eslint needs some time to grow too.

@Bobgy
Copy link
Contributor Author

Bobgy commented Oct 18, 2019

One remaining issue is object-literal-sort-keys rule:
It is super painful to maintain manually, however, there is no auto fix that can guarantee no runtime difference.
If we switch to eslint, we probably can use https://www.npmjs.com/package/eslint-plugin-sort-keys-fix to do an unsafe auto fix.

but I don't quite like this rule anyway. There are cases that I prefer a manual ordering for example css properties:

{
  width: 100,
  height: 100,
  color: 'red',
}

@Bobgy
Copy link
Contributor Author

Bobgy commented Oct 23, 2019

Update, after making lint errors as warnings during development. object-literal-sort-keys is less an issue for me.

My workflow becomes:

  1. make changes and test
  2. before pushing to remote, fix all lint errors

So it's more a problem of whether we want this rule or not. I will leave that to future discussion in a separate issue.

/close

@k8s-ci-robot
Copy link
Contributor

@Bobgy: Closing this issue.

In response to this:

Update, after making lint errors as warnings during development. object-literal-sort-keys is less an issue for me.

My workflow becomes:

  1. make changes and test
  2. before pushing to remote, fix all lint errors

So it's more a problem of whether we want this rule or not. I will leave that to future discussion in a separate issue.

/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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants