-
-
Notifications
You must be signed in to change notification settings - Fork 726
feat(linter/plugins): Token-related SourceCode APIs (TS ESLint implementation)
#15861
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?
feat(linter/plugins): Token-related SourceCode APIs (TS ESLint implementation)
#15861
Conversation
SourceCode APIs (TS ESLint implementation)
3e3a5bb to
dd03ef0
Compare
Cameron and I had a bit of an argument about exactly this question! We concluded in the end to keep all the deprecated methods, to maximize compatibility with older plugins, which may take some time to get updated (or in some cases, will never get updated). Our rationale is that most of these methods are just aliases, so no maintenance burden to keep them. The only one which is slightly different from its non-deprecated "brother" is
That's true, but even actively developed projects may use old unmaintained plugins. Note: I added the stubs in #15645. That PR also added tests which illustrate the difference in behavior between |
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.
Possibly premature review, as it's still marked as draft, but I'm keen to get it merged so I thought I'd go through it now.
Apart from the comments below, this looks good to me. Sorry for the volume of comments - most are pretty small details.
Once we're happy with getTokens impl, I think we should merge this, and we can add more methods in separate PRs. No need to do the whole API in a single PR.
A couple of points which we can also leave to follow-ups:
-
We should ideally lazy-load
@typescript-eslint/typescript-estreepackage only whengetTokensis first called. -
I assume TS-ESLint's parser also generates a
ScopeManager. If it does, we may as well cache it, to avoid running scope analysis again if plugin doessourceCode.getTokens()followed by gettingsourceCode.scopeManager.
176a184 to
db8abc2
Compare
db8abc2 to
0d4164e
Compare
0d4164e to
bc17b6d
Compare
The former delegates to the latter for parsing and lexing. I decided to use |
faf3560 to
de1f77e
Compare
…pshot (#15906) Contents of this test snapshot depends on the constructor names of scope class instances. This is a little fragile, as they can change depending on how the code is bundled (see #15861 (comment)). At some point, we'll full minify the bundle, at which point class names will be come random gibberish like `e`, `t`, `aZ`, etc. Avoid this problem by outputting `type` field of scope objects instead.
Ah ha that makes perfect sense. I had misunderstood. |
e23e179 to
173e5a9
Compare
9fb3811 to
ab159d9
Compare
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 implements token-related SourceCode APIs for the oxlint JavaScript plugin system, specifically the getTokens method. The implementation uses @typescript-eslint/typescript-estree to parse source code and extract tokens, providing compatibility with ESLint's token API.
Key changes:
- Implements
sourceCode.getTokens()method with support for filters, comment inclusion, and before/after token counts - Defines comprehensive TypeScript token types (13 token types including Boolean, Keyword, Punctuator, JSXIdentifier, etc.)
- Adds
typescriptand@typescript-eslint/typescript-estreeas runtime dependencies
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds lockfile entries for @typescript-eslint/typescript-estree and its dependencies |
| npm/oxlint/package.json | Adds typescript as a production dependency for the published package |
| apps/oxlint/package.json | Moves typescript to dependencies and adds typescript-estree as devDependency |
| apps/oxlint/src-js/plugins/tokens.ts | Implements getTokens() with binary search and token type definitions |
| apps/oxlint/src-js/plugins/types.ts | Removes duplicate Token interface, imports from tokens.ts instead |
| apps/oxlint/src-js/plugins/source_code.ts | Integrates resetTokens() into cleanup logic |
| apps/oxlint/src-js/index.ts | Exports all token-related types |
| apps/oxlint/test/tokens.test.ts | Unit tests for getTokens() based on ESLint test cases |
| apps/oxlint/test/fixtures/sourceCode_token_methods/* | E2E test fixture demonstrating token API usage |
| apps/oxlint/test/e2e.test.ts | Adds E2E test for token methods |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/oxlint/test/fixtures/sourceCode_token_methods/files/eslint-test-case.js
Show resolved
Hide resolved
overlookmotel
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.
Apart from the small comments below, looks good!
| // TODO: this test is failing because of a difference in program range between `espree` and `@typescript-eslint/typescript-estree`. | ||
| // Unskip after integrating `espree`. |
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 is a really weird one. ESLint is changing it's behavior in ESLint 10 to make the range of Program be [0, <length of source text>] (no matter whether there's leading/trailing comments/whitespace or not). TS-ESLint plans to follow suit.
https://eslint.org/blog/2025/10/whats-coming-in-eslint-10.0.0/#updates-to-program-ast-node-range-coverage
typescript-eslint/typescript-eslint#11026 (comment)
Oxc will make the change too, and everyone will be aligned (at last).
So I suggest modifying these tests and unskipping them by either:
const Program = { range: [5, 36] } as Node;(to match current Espree). or- Manually add the extra tokens to the array below, so the tests pass.
Either is fine to do, I think. Just need to make a comment explaining why.
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.
PS Thanks for putting in the unglamorous work of upping the test coverage. Appreciate it.
| /** | ||
| * Number of following tokens to additionally return. | ||
| */ | ||
| afterCount = typeof afterCount === 'number' ? afterCount : 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.
According to the JSDoc comment on this function (which is lifted from ESLint), params are either:
(countOptions: CountOptions | FilterFn)or(beforeCount?: number, afterCount?: number)
That would mean that afterCount param should be ignored if countOptions is an object or function.
Does ESLint's implementation follow the JSDoc comment, or not actually do what it says it does? I guess we should do whatever ESLint does, but if behavior is contrary to the doc comment, it'd be good to add a comment explaining why.
Or we could open a bug report on ESLint.
Sorry, it really is insanely anal of me to question this!
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.
Sorry, it really is insanely anal of me to question this!
YOU WORK FULL TIME ON A LINTER! It's part of the job description 😭.
Looking into it.
ab159d9 to
e1202dd
Compare
SourceCodeAPIs #14829 (comment)@typescript-eslint/typescript-estreefor tokens. The source code is fully parsed and everything but the tokens is discarded.typescript-eslintproject and copied over with only formatting edits.Tasks
getTokenssrc-js/plugins/tokens.ts__filenameusage.Uncaught ReferenceError: __filname is not defined.Prevent class name mangling bytest(linter/plugins): avoid using scope constructor names in test snapshot #15906 (review)tsdown. Could be deferred.eslintandtypescript-eslint. Could be deferred.Future work
getFirstTokengetFirstTokensgetLastTokengetLastTokensgetTokenBeforegetTokenOrCommentBeforegetTokensBeforegetTokenAftergetTokenOrCommentAftergetTokensAftergetTokensBetweengetFirstTokenBetweengetFirstTokensBetweengetLastTokenBetweengetLastTokensBetweengetTokenByRangeStartisSpaceBetweenisSpaceBetweenTokensDecisions
@typescript-eslint/typescript-estreepeer-depends ontypescript. How should we package it? Currently,typescriptis being made an optional dependency.getTokenOrCommentBefore,getTokenOrCommentAfter, andisSpaceBetweenTokens. These are surface level deprecations: the functionality was merged with other methods (theincludeComments: trueoption) and plugins can migrate with a one line change. I'm guessing we are targeting fairly modern, actively developed projects. Should we expose them?