-
-
Notifications
You must be signed in to change notification settings - Fork 221
feat: add types to ESLint Scope #709
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
base: main
Are you sure you want to change the base?
Conversation
| /** | ||
| * @fileoverview This file contains the types for ESLint Scope. | ||
| * It was initially extracted from the DefinitelyTyped repository. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adapting the DefinitelyTyped types should be fine in terms of licensing, although I think it doesn't quite align with this project's guidelines:
| - You will only Submit Contributions where You have authored 100% of the content. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds built-in TypeScript type definitions to the ESLint Scope package, eliminating the need for separate @types/eslint-scope packages. The types were adapted from DefinitelyTyped with enhancements including nullable global scope, additional scope subclasses, deprecated member tags, and improved type safety through type checking of JavaScript source files.
Key changes:
- Added TypeScript declaration files (index.d.ts, index.d.cts) with comprehensive type definitions
- Enabled type checking for JavaScript source files using JSDoc annotations
- Added type testing infrastructure with npm scripts and CI workflow
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/eslint-scope/tsconfig.json | TypeScript configuration for type-checking JavaScript source files |
| packages/eslint-scope/tests/types/tsconfig.json | TypeScript configuration for type test files |
| packages/eslint-scope/tests/types/types.test.ts | Comprehensive type tests covering all exported APIs |
| packages/eslint-scope/tests/types/cjs-import.test.cts | CommonJS import type test |
| packages/eslint-scope/package.json | Added type exports, dependencies, and scripts |
| packages/eslint-scope/lib/*.js | Added JSDoc type annotations to source files |
| packages/eslint-scope/lib/index.d.ts | ESM type declaration file (re-exports from .cts) |
| packages/eslint-scope/lib/index.d.cts | Main CommonJS type declaration file |
| eslint.config.js | Added TypeScript file linting configuration |
| package.json | Added TypeScript parser and expect-type plugin |
| .github/workflows/ci.yml | Added "Are the types wrong?" CI job |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * The identifier node of the reference. | ||
| */ | ||
| identifier: ESTree.Identifier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can also be JSXIdentifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true, but we need to update the base interface Scope.Reference in ESLint first, because we can't broaden the type of the identifier member in the implementing class.
js/packages/eslint-scope/lib/index.d.cts
Line 568 in c6d38b2
| export class Reference implements eslint.Scope.Reference { |
|
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
lumirlumir
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good start!
I've left some comments throughout, especially regarding documentation updates.
| "@arethetypeswrong/cli": "^0.18.2", | ||
| "@typescript-eslint/parser": "^8.7.0", | ||
| "chai": "^6.0.0", | ||
| "eslint": ">=10.0.0-alpha.0 <10.0.0 || ^10.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "eslint": ">=10.0.0-alpha.0 <10.0.0 || ^10.0.0", | |
| "eslint": ">=10.0.0-alpha.0 <10.0.0 || ^10.0.1", |
It seems that @eslint/[email protected] was released accidentally, so perhaps eslint should also start at 10.0.1 instead of 10.0.0?
References:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer need to keep the version numbers of eslint and @eslint/js in sync. In fact, eslint no longer depends on @eslint/js. See also https://github.com/eslint/tsc-meetings/blob/main/notes/2025/2025-11-13.md#should-we-continue-releasing-eslintjs-every-time-we-release-eslint. The next stable ESLint release will be v10.0.0.
| /** | ||
| * Represents a variable in a scope. | ||
| */ | ||
| export class Variable<TReference extends Reference = Reference> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: there are some static properties like the following:
Should these types also be applied to the Variable class?
js/packages/eslint-scope/lib/variable.js
Lines 77 to 83 in 26eb6e2
| Variable.CatchClause = "CatchClause"; | |
| Variable.Parameter = "Parameter"; | |
| Variable.FunctionName = "FunctionName"; | |
| Variable.ClassName = "ClassName"; | |
| Variable.Variable = "Variable"; | |
| Variable.ImportBinding = "ImportBinding"; | |
| Variable.ImplicitGlobalVariable = "ImplicitGlobalVariable"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. These are the possible values for the type property of aDefinition. These constants aren't documented, but neither are the constructor signatures for Scope, Variable, Definition, etc, which all have their type definitions. I'd say it's fine to add types in this case, but I don't feel strongly.
| /** | ||
| * Represents a reference to a variable. | ||
| */ | ||
| export class Reference { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: there are also some static properties like the following:
Should these types also be applied to the Reference class?
js/packages/eslint-scope/lib/reference.js
Lines 146 to 162 in 26eb6e2
| /** | |
| * @constant Reference.READ | |
| * @private | |
| */ | |
| Reference.READ = READ; | |
| /** | |
| * @constant Reference.WRITE | |
| * @private | |
| */ | |
| Reference.WRITE = WRITE; | |
| /** | |
| * @constant Reference.RW | |
| * @private | |
| */ | |
| Reference.RW = RW; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with the previous question, these values are undocumented, so I don't feel strongly whether they should be included in the types or not.
Co-authored-by: 루밀LuMir <[email protected]>
Co-authored-by: 루밀LuMir <[email protected]>
|
Okay, I think I've addressed all the feedback now. |
Prerequisites checklist
What is the purpose of this pull request?
Add built-in types to ESLint Scope
What changes did you make? (Give an overview)
addGlobals()toScopeManagerglobalScopeinScopeManageras nullableimplicitfromScopetoGlobalScopeand addedimplicit.variablesScopesubclasses@deprecatedtag to deprecated members per https://github.com/eslint/eslint/blob/v10.0.0-alpha.0/docs/src/extend/scope-manager-interface.mdfunctionExpressionScopetotrueorfalsedepending on whether theScopeisFunctionExpressionNameScoperesolve()inScope(the definition in@types/eslint-scopeis very different, possibly unrelated)PatternVisitorpackages/eslint-scope/libto verify they match the type declarationsReference#isStatic()now always returns a boolean even whenresolvedis not setScopeManager#isImpliedStrict()now always returns a boolean even when theimpliedStrictoption is not setRelated Issues
fixes #708
Is there anything you'd like reviewers to focus on?