kubeflow / metadata

Repository for assets related to Metadata.
Apache License 2.0
121 stars 69 forks source link

[frontend] Switch to TSLint #154

Open kwasi opened 4 years ago

kwasi commented 4 years ago

/area frontend /kind feature

Background: We have a tslint.json but lint with eslint due to react-scripts webpack

I added some generated protocol buffers in 13794fc and broke frontend webpack compile due to lint failures for the no-undef rule. I was thrown off by the fact that there is a tslint.json configuration file but the failures were an eslint rule.

Adding the generated file path to an eslintIgnore block in package.json didn’t work and adding the path to tslint.json didn’t fix the webpack compile either.

How does eslint get run?

When scripts run by create-react-app launch webpack the linting phase uses eslint-loader with the eslint-config-react-app configuration [1], which uses @typescript-eslint to provide linting rules for TypeScript files to ESlint [2] without adding a dependency on tslint.

This configuration also ignores any eslintIgnore or eslintConfig blocks in package.json unless the EXTEND_ESLINT environment variable is set [3].

Proposal: Modify the project to use TSLint through react-scripts-ts

Tasks for implementation:

Caveats

There are a lot of existing files in the codebase that don’t match the rules declared in tslint.json, so adding tslint will break the webpack compile until the lint errors are fixed. If we do this in conjunction with using adding prettier for code styles (https://github.com/kubeflow/metadata/issues/141), we can do both as one large-scale reformatting and reduce the merge overhead.

References

[1] https://github.com/facebook/create-react-app/blob/6d6dfa9ba5a6042ab5690fe2a38b3ee54e644c9f/packages/react-scripts/config/webpack.config.js#L349

[2] https://github.com/facebook/create-react-app/blob/6d6dfa9ba5a6042ab5690fe2a38b3ee54e644c9f/packages/eslint-config-react-app/index.js#L57

[3] https://github.com/facebook/create-react-app/blob/6d6dfa9ba5a6042ab5690fe2a38b3ee54e644c9f/packages/react-scripts/config/webpack.config.js#L359

[4] https://github.com/jpavon/react-scripts-ts/blob/d80cda5b72d17aeb217b23b331d9eba225319b3c/config/paths.js#L62

k8s-ci-robot commented 4 years ago

@kwasi: The label(s) area/frontend cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other

In response to [this](https://github.com/kubeflow/metadata/issues/154): >/area frontend >/kind feature > >### Background: We have a `tslint.json` but lint with `eslint` due to `react-scripts` webpack > >I added some generated protocol buffers in 13794fc and broke frontend `webpack` compile due to lint failures for the [no-undef](https://eslint.org/docs/rules/no-undef) rule. I was thrown off by the fact that there is a `tslint.json` configuration file but the failures were an `eslint` rule. > >Adding the generated file path to an `eslintIgnore` block in `package.json` didn’t work and adding the path to `tslint.json` didn’t fix the webpack compile either. > >#### How does `eslint` get run? > >When scripts run by `create-react-app` launch `webpack` the linting phase uses `eslint-loader` with the `eslint-config-react-app` configuration [1], which uses @typescript-eslint to provide linting rules for TypeScript files to ESlint [2] without adding a dependency on `tslint`. > >This configuration also ignores any `eslintIgnore` or `eslintConfig` blocks in `package.json` unless the EXTEND_ESLINT environment variable is set [3]. > >### Proposal: Modify the project to use TSLint through `react-scripts-ts` > >* Using `tslint` will prevent programmer error when trying to make lint changes in `tslint.json` >* `react-scripts-ts` uses `tslint` directly instead of `eslint` with TypeScript support [4] >* `react-scripts-ts` doesn't require environment variables to apply TSLint config >* Supports aligning frontend development practices with `kubeflow/pipelines` which uses `tslint` > >### Tasks for implementation: > >* [ ] Switch `react-scripts` to `react-scripts-ts` in `package.json` to pick up `tslint.json` >* [ ] Remove eslint configuration lines from package.json >* [ ] Format the codebase using `tslint` > >#### Caveats > >There are a lot of existing files in the codebase that don’t match the rules declared in `tslint.json`, so adding `tslint` will break the webpack compile until the lint errors are fixed. If we do this in conjunction with using adding prettier for code styles (https://github.com/kubeflow/metadata/issues/141), we can do both as one large-scale reformatting and reduce the merge overhead. > >## References > >[1] https://github.com/facebook/create-react-app/blob/6d6dfa9ba5a6042ab5690fe2a38b3ee54e644c9f/packages/react-scripts/config/webpack.config.js#L349 > >[2] https://github.com/facebook/create-react-app/blob/6d6dfa9ba5a6042ab5690fe2a38b3ee54e644c9f/packages/eslint-config-react-app/index.js#L57 > >[3] >https://github.com/facebook/create-react-app/blob/6d6dfa9ba5a6042ab5690fe2a38b3ee54e644c9f/packages/react-scripts/config/webpack.config.js#L359 > >[4] https://github.com/jpavon/react-scripts-ts/blob/d80cda5b72d17aeb217b23b331d9eba225319b3c/config/paths.js#L62 > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
kwasi commented 4 years ago

/area front-end

jtfogarty commented 4 years ago

/priority p2