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

Move samples and template from TSLint to ESLint #561

Merged
merged 15 commits into from Nov 25, 2019
Merged

Move samples and template from TSLint to ESLint #561

merged 15 commits into from Nov 25, 2019

Conversation

fer22f
Copy link
Contributor

@fer22f fer22f commented Oct 26, 2019

Issue

TSLint is being deprecated (palantir/tslint#4534), and this PR fixes #555.

Solution and steps

  1. Convert tslint.json to .eslint.js
  2. Change mentions in the docs
  3. Use ESLint and related plugins instead of TSLint in package.json
  4. Change samples to be correctly linted under the new rules

Checklist

  • Add/update/check docs (code comments and docs/ folder).
  • Add/update/check tests.
  • Update/check the cli generators.
  • Check that npm run lint succeeds in each sample.
  • Search for all mentions of tslint, eslint and parseInt in the repository.
  • Check that all the new dependencies are required.
  • Generate a project with the CLI.
  • In the new project, test that the commands npm run lint and npm run lint:fix work.
  • Check that VSCode extension works as the documentation suggests.
  • Check that migration files are ignored.

@fer22f
Copy link
Contributor Author

fer22f commented Oct 26, 2019

Is this .eslint.js set of rules okish? This is 100% tweakable, I just did what felt right, but maybe we could use a more strict set of rules.

@LoicPoullain LoicPoullain added this to Work In Progress in Issue tracking via automation Nov 4, 2019
@LoicPoullain LoicPoullain self-requested a review November 4, 2019 08:50
Copy link
Member

@LoicPoullain LoicPoullain left a comment

Choose a reason for hiding this comment

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

Sorry it took so long. I'm a little overwhelmed these weeks.

Thank you for sharing your changes in different commits by the way. It really makes the review easier. 👍

I recently changed Foal CI from Travis to Github Actions, this is why no tests are running in the web hooks. We have to run them manually on local.

Review steps

  • Review each commit. ✅
  • Clone locally the fork. ✅
  • Check that npm run lint succeeds in each sample. ✅
  • Run all unit and acceptance tests. ✅
  • Search for all mentions of tslint, eslint and parseInt in the repository. ✅
  • Check that all the new dependencies are required. ✅
  • Read the documentation of the dependencies. ✅
  • Generate a project with the CLI. 🔴

When building @foal/cli, the command copyfiles does not copy by default the templates starting with a dot. We have to specify it manually by adding the flag -a in packages/cli/package.json -> copyfiles -a -u 3.

  • In the new project, test that the commands npm run lint and npm run lint:fix work. ✅
  • Check that VSCode extension works as the documentation suggests. 🔴 

We need to tell users to extend their settings here to make it work (to support both TypeScript and the auto-fix feature):

  "eslint.validate": [
    { "language": "typescript", "autoFix": true }
  ],
  "eslint.autoFixOnSave": true
  • Check that migration files are ignored. 🔴

Migrations are generated with different coding rules. We should ignore them by default not to get lint errors when getting started with Foal. Developers will still be able to change this if they want to after.

To do so, we can add this in the package.json templates: "eslintIgnore": [ "src/migrations/*.ts" ]

  • Run e2e_test.sh locally. 🔴

Generated entities do not pass the linting.

Do not use "// @ts-ignore" comments because they suppress compilation errors.
'Column' is defined but never used

We need to replace all // @ts-ignore : 'Column' is declared but its value is never read. with // eslint-disable-next-line @typescript-eslint/no-unused-vars.

  • Check that the files eslintrc.js and tslint.json are equivalent. ✔️

I took a look at the rules that you migrated from TSLint as well as the others that you added to « fix » the recommended ones from ESLint. All seems good. Since the tests are passing, I think we can reasonably consider that both files are equivalent.

Good work! There are some stuff to change/add and we’re good! 👍

@fer22f
Copy link
Contributor Author

fer22f commented Nov 17, 2019

I think I will wait until eslint/eslint#12274 is merged so don't need one more .eslintignore or cluttering of package.json to ignore migrations.

@fer22f
Copy link
Contributor Author

fer22f commented Nov 23, 2019

Ok, it has landed! ESLint 6.7.0 has been released :D

@LoicPoullain LoicPoullain changed the base branch from master to v1-3-0 November 25, 2019 15:16
Copy link
Member

@LoicPoullain LoicPoullain left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Great idea to use the .eslintrc file to ignore migrations!

@LoicPoullain LoicPoullain mentioned this pull request Nov 25, 2019
9 tasks
@LoicPoullain LoicPoullain merged commit 7c47652 into FoalTS:v1-3-0 Nov 25, 2019
Issue tracking automation moved this from Work In Progress to Done / Closed This Release Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Issue tracking
  
Done / Closed This Release
Development

Successfully merging this pull request may close these issues.

None yet

3 participants