refactor: first step toward ESLint 10 support#454
Conversation
🦋 Changeset detectedLatest commit: 742e0d2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to a58501c in 6 seconds. Click for details.
- Reviewed
101lines of code in3files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_TVOcxA287xjliOKs
You can customize 
📝 WalkthroughWalkthroughA changeset adds a patch note for ESLint 10 support. Parser resolution in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/parse.ts (1)
188-196:⚠️ Potential issue | 🔴 CriticalBug:
typeof parsereferences the module-level export, notparser.parse.On line 191,
typeof parse === 'function'resolves to theparsefunction exported at line 71 of this file — it's alwaystrue. The intended check istypeof parser.parse === 'function', consistent with theparseForESLintcheck on line 193.This means any object with a
parseproperty (regardless of type) passes validation, which could lead to a runtime error later ifparser.parseisn't actually callable.🐛 Proposed fix
if ( parser && typeof parser !== 'string' && - (('parse' in parser && typeof parse === 'function') || + (('parse' in parser && typeof parser.parse === 'function') || ('parseForESLint' in parser && typeof parser.parseForESLint === 'function')) ) {
There was a problem hiding this comment.
Pull request overview
This PR takes the first step toward ESLint 10 support by refactoring how the plugin accesses parser configuration, specifically addressing a critical crash bug (#452) and preparing for the removal of context.parserPath in ESLint 10.
Changes:
- Fixed TypeError crash when
context.parserOptionsis undefined by adding proper existence guards and prioritizinglanguageOptions(flat config) over legacyparserOptions - Consolidated
getParserandgetParserPathinto a singlegetParserOrPathfunction with proper guards forcontext.parserPathaccess - Renamed
pathimport tonodePathto avoid naming conflicts with function parameters
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/utils/source-type.ts | Refactored to prioritize languageOptions over parserOptions with proper guards, fixing the crash from issue #452 |
| src/utils/parse.ts | Merged parser resolution logic into getParserOrPath, added guards for context.parserPath, and renamed path import to avoid conflicts |
| .changeset/some-moments-trade.md | Added changeset documenting this as a patch release for ESLint 10 support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 742e0d2 in 9 seconds. Click for details.
- Reviewed
122lines of code in3files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_9n1Cq20mQuklRvRV
You can customize
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/parse.ts (1)
186-196: Parser object validation looks correct but consider a small readability improvement.The validation checks both
parseandparseForESLintmethods, matching the ESLint parser contract. The condition on lines 188–194 is somewhat dense with the mixedinchecks andtypeofguards.Consider a minor restructuring for readability:
Optional: extract a helper for parser validation
+function isParserModule( + parser: unknown, +): parser is TSESLint.Parser.ParserModule { + return ( + parser != null && + typeof parser !== 'string' && + (('parse' in parser && typeof (parser as Record<string, unknown>).parse === 'function') || + ('parseForESLint' in parser && + typeof (parser as Record<string, unknown>).parseForESLint === 'function')) + ) +}This would simplify the condition in
getParserOrPathand could be reused if parser validation is needed elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/parse.ts` around lines 186 - 196, The parser validation condition is dense and should be extracted for readability: create a helper function (e.g., isValidParser) that accepts the parser value and returns true if it's a non-string object implementing either a callable parse or parseForESLint method, then replace the inlined conditional inside getParserOrPath (the block that checks the parser const) with a call to isValidParser(parser) and cast/return as TSESLint.Parser.ParserModule when true; keep the same runtime checks (non-string, 'parse' in parser && typeof parse === 'function', or 'parseForESLint' in parser && typeof parseForESLint === 'function') so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/parse.ts`:
- Around line 186-196: The parser validation condition is dense and should be
extracted for readability: create a helper function (e.g., isValidParser) that
accepts the parser value and returns true if it's a non-string object
implementing either a callable parse or parseForESLint method, then replace the
inlined conditional inside getParserOrPath (the block that checks the parser
const) with a call to isValidParser(parser) and cast/return as
TSESLint.Parser.ParserModule when true; keep the same runtime checks
(non-string, 'parse' in parser && typeof parse === 'function', or
'parseForESLint' in parser && typeof parseForESLint === 'function') so behavior
is unchanged.
Fixes #452 and #455 , and a step toward #438.
sourceTypepreferscontext.languageOptionswhen possiblecontext.parserOptionsbysourceTypeis now guarded, which fixes TypeError in @import/unambiguous crashes linter #452 and ESLint 10: import-x/unambiguous parser-context crash #455getParserandgetParserPathimpl into onegetParserOrPathImportant
Refactor for ESLint 10 support by updating
sourceTypedetermination and merging parser functions inparse.tsandsource-type.ts.sourceTypeinsource-type.tsnow preferscontext.languageOptionsovercontext.parserOptionsfor ESLint 10 compatibility.context.parserOptionsis now guarded to prevent crashes with ESLint 10.getParserandgetParserPathintogetParserOrPathinparse.tsto streamline parser selection logic.pathtonodePathinparse.tsto avoid conflicts.This description was created by
for 742e0d2. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Chores
Refactor