Skip to content

tools: lint typescript files #55709

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

Closed
wants to merge 4 commits into from
Closed

Conversation

avivkeller
Copy link
Member

Fixes #55702 by enabling typescript-eslint on TypeScript files.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Nov 3, 2024
Copy link

codecov bot commented Nov 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (f38cefa) to head (e46dd39).
Report is 985 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55709      +/-   ##
==========================================
- Coverage   88.40%   88.40%   -0.01%     
==========================================
  Files         654      654              
  Lines      187746   187815      +69     
  Branches    36126    36134       +8     
==========================================
+ Hits       165978   166038      +60     
+ Misses      15009    15005       -4     
- Partials     6759     6772      +13     

see 44 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@avivkeller avivkeller added the strip-types Issues or PRs related to strip-types support label Nov 3, 2024
Copy link
Member

@marco-ippolito marco-ippolito left a comment

Choose a reason for hiding this comment

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

I checkout on the pr, changed a typescript file with invalid format, and run make lint-js-fix and it did not fix the format.

@avivkeller
Copy link
Member Author

avivkeller commented Nov 6, 2024

Where is the TypeScript file you edited? In my case, it appears to be correctly linting TypeScript files.

Most places where TypeScript files exist aren't linted, for example, the test fixtures are excluded from linting entirely

@avivkeller
Copy link
Member Author

avivkeller commented Nov 8, 2024

typescript-eslint can also replace the Babel parser (removing 44 deps from our ESLint configuration), should I do that here, or in a follow up?

@marco-ippolito
Copy link
Member

marco-ippolito commented Nov 8, 2024

typescript-eslint can also replace the Babel parser (removing 44 deps from our ESLint configuration), should I do that here, or in a follow up?

Follow up would be better
I'm surprised there is no linting error 🤔

@marco-ippolito
Copy link
Member

On which typescript files have you tested it?
It's not linting the typings folder.
Screenshot 2024-11-08 at 09 03 33

I think I would actually create a command make lint-ts and run it also in the test/fixtures/typescript, typing folder and examples with the ts extension. Otherwise it seems not doing anything

@avivkeller
Copy link
Member Author

avivkeller commented Nov 8, 2024

test/fixtures/typescript

That folder contains a lot of lint errors (for example, unreachable code), as with most of the fixtures, and I think it's best to not lint.

typings

I'll edit the config to lint that folder, now that there is typescript support.

I think I would actually create a command make lint-ts

IMO keeping it as js is okay, and if something changes, there can always be a follow-up.

@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2024
@nodejs-github-bot
Copy link
Collaborator

@marco-ippolito
Copy link
Member

@avivkeller are you still interested in landing this PR?

@avivkeller
Copy link
Member Author

It's been several months, and I no longer have the bandwidth to continue with this, apologies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. strip-types Issues or PRs related to strip-types support tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tools: lint TypeScript files
4 participants