Skip to content

feat: Add types#18854

Merged
fasttime merged 8 commits intomainfrom
issue18845
Sep 6, 2024
Merged

feat: Add types#18854
fasttime merged 8 commits intomainfrom
issue18845

Conversation

@nzakas
Copy link
Copy Markdown
Member

@nzakas nzakas commented Sep 4, 2024

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Added the type definitions from @types/eslint to the project:

  • Added all type definitions in lib/types
  • Added tests in tests/lib/types
  • Added new test:types script to run type checking tests
  • Added npm run test:types to CI to be run on each PR
  • Added types entries to package.json

For simplicity, I did not enable type checking for the whole repo, just for the type tests.

This is intended to be a base for us to build off of. We can look to make improvements in future PRs.

fixes #18845

Is there anything you'd like reviewers to focus on?

The types are copied over as-is without modification (aside from including the license). We don't need to review their contents right now.

Does the testing setup make sense?

@nzakas nzakas requested a review from a team as a code owner September 4, 2024 16:20
@eslint-github-bot eslint-github-bot Bot added the feature This change adds a new feature to ESLint label Sep 4, 2024
@netlify
Copy link
Copy Markdown

netlify Bot commented Sep 4, 2024

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit a5beaf2
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/66db14c73744780008fdf176

@nzakas nzakas added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Sep 5, 2024
Comment thread eslint.config.js
"**/test.js",
".vscode"
".vscode",
"**/*.ts"
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Just for now. 😄

@nzakas
Copy link
Copy Markdown
Member Author

nzakas commented Sep 5, 2024

Looks like the webpack plugin is using an older version of @types/eslint, but still seems like there may be an error in the types for formatters?

@fasttime
Copy link
Copy Markdown
Member

fasttime commented Sep 5, 2024

I can confirm the Formatter type is broken, see #18821 (comment). Would patching that type help to fix the error?

@nzakas
Copy link
Copy Markdown
Member Author

nzakas commented Sep 5, 2024

Yes, I think that's all it would take to make both integration tests pass. I'll give it a shot.

@nzakas
Copy link
Copy Markdown
Member Author

nzakas commented Sep 6, 2024

Okay, I fixed the incorrect type and we now have three integration tests with other projects.

@fasttime fasttime added the accepted There is consensus among the team that this change meets the criteria for inclusion label Sep 6, 2024
Comment thread lib/types/index.d.ts
}

interface Formatter {
format(results: LintResult[], data?: LintResultData): string | Promise<string>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The second argument of LoadedFormatter#format() is just an object with an optional maxWarningsExceeded property:

* `format` (`(results: LintResult[], resultsMeta: ResultsMeta) => string | Promise<string>`)<br>
The method to convert the [LintResult] objects to text. `resultsMeta` is an object that will contain a `maxWarningsExceeded` object if `--max-warnings` was set and the number of warnings exceeded the limit. The `maxWarningsExceeded` object will contain two properties: `maxWarnings`, the value of the `--max-warnings` option, and `foundWarnings`, the number of lint warnings.

The cwd and rulesMeta properties are passed directly to the underlying formatter function:

eslint/lib/eslint/eslint.js

Lines 1245 to 1255 in 9bde90c

return formatter(results, {
...resultsMeta,
cwd,
get rulesMeta() {
if (!rulesMeta) {
rulesMeta = eslint.getRulesMetaForResults(results);
}
return rulesMeta;
}
});

Since that type has been broken for so long and the integration tests are passing now, we can merge this change as it is, and I will try to sort up things in a follow-up PR.


sourceCode.getAllComments();

sourceCode.getJSDocComment(AST); // $ExpectType Comment | null
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

$ExpectType annotations are not working any more. We should also fix this in a follow-up.

@fasttime fasttime removed the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Sep 6, 2024
Copy link
Copy Markdown
Member

@fasttime fasttime left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I'm going to merge this PR now so that it gets included in the ESLint v9.10.0 release.

@fasttime fasttime merged commit 301b90d into main Sep 6, 2024
@fasttime fasttime deleted the issue18845 branch September 6, 2024 19:56
@sam3k
Copy link
Copy Markdown
Contributor

sam3k commented Sep 8, 2024

Per 2024-09-05 TSC meeting, this was released early and we will await feedback from users to make sure it is working as expected.

@nzakas added some integration tests for the types.

@dimitropoulos
Copy link
Copy Markdown
Contributor

thank you for doing this!

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

Labels

accepted There is consensus among the team that this change meets the criteria for inclusion feature This change adds a new feature to ESLint github actions

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

Change Request: Publish types

4 participants