feat: introduce oxc as parser#503
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughReplace bespoke Acorn import detection with a shared ESTree detector factory, add oxc-parser support and routing, move scope/virtual-import logic into a new ESTree module, update types and docs, adjust dependency manifests, and replace Acorn-only tests with a unified ESTree test suite for both parsers. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Detector as ESTreeDetector
participant Parser
participant Traverser
participant Context as UnimportContext
participant Magic as MagicString
Caller->>Detector: invoke(code, ctx, options)
Detector->>Parser: parse(code) -> AST
Parser-->>Detector: AST
Detector->>Traverser: traverse AST -> scopes & unmatched ids
Traverser-->>Detector: unmatched identifiers
Detector->>Context: load import map & virtualImports
Context-->>Detector: import map / addons
Detector->>Magic: remove virtual import ranges / apply edits
Detector-->>Caller: return { s: MagicString, strippedCode, matchedImports, isCJSContext:false, firstOccurrence:0 }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #503 +/- ##
==========================================
- Coverage 98.73% 94.69% -4.04%
==========================================
Files 14 15 +1
Lines 1817 1018 -799
Branches 374 337 -37
==========================================
- Hits 1794 964 -830
- Misses 23 50 +27
- Partials 0 4 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
package.json (1)
37-39: Use a bounded peer range foroxc-parserinstead of"*".
"*"can pull incompatible API changes and breakparser: 'oxc'at runtime. Please align this with the tested catalog version range (for example^0.116.0).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 37 - 39, The peerDependencies entry currently pins "oxc-parser" to "*", which can introduce breaking API changes at install time; update the package.json peerDependencies object to use a bounded semver range that matches the tested catalog (e.g. "oxc-parser": "^0.116.0") instead of "*", and run install/tests to verify parser: 'oxc' still works; locate the peerDependencies object in package.json and replace the "*" for "oxc-parser" with the appropriate caret range.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/detect-estree.ts`:
- Around line 62-64: The current stub returns constant metadata (isCJSContext:
false, firstOccurrence: 0) which must be replaced with real detection: inspect
the parsed ESTree (AST and tokens) in detect-estree.ts to determine whether the
file uses CommonJS patterns and the index/position of the first occurrence, then
set isCJSContext accordingly and compute firstOccurrence from the AST/token
location (e.g., first require/module.exports/exports usage or top-level CommonJS
node). Update the code paths that build and return the metadata object so they
populate real values for isCJSContext and firstOccurrence instead of the
hardcoded defaults.
- Around line 163-210: The Identifier handling block misses statement contexts
like return, throw, and for-loop tests; update the switch in the 'Identifier'
case to add cases for 'ReturnStatement' and 'ThrowStatement' (check
parent.argument === node then call scopeCurrent.references.add(node.name)) and
for 'ForStatement' (check parent.test === node then add), keeping the existing
pattern used for If/While/DoWhile and Switch; ensure you use the same guard
style (parent.<property> === node) and call
scopeCurrent.references.add(node.name) in each new case.
- Around line 151-155: The function parameters are being added to the outer
scope because the code iterates parent.params and calls
scopeCurrent.declarations.add(id.name) before creating the function scope; move
the parameter declaration logic to after pushScope(node) (or add them to the
newly created scope object returned/assigned by pushScope) so
parameterIdentifiers (derived from parent.params) are added to the function's
own scope rather than the outer scope; update the block that uses parent.params,
parameterIdentifiers and scopeCurrent.declarations.add to reference the new
scope created by pushScope(node).
In `@src/detect.ts`:
- Around line 13-14: When handling case 'oxc' in detect.ts, wrap the dynamic
import(import('./detect-oxc')) in a try/catch so a missing optional dependency
yields a clear error message; specifically, await import('./detect-oxc') inside
an async block and on catch check the error (e.g., error.code ===
'ERR_MODULE_NOT_FOUND' or error.message includes 'oxc-parser') and throw a new
Error telling the user to install the optional "oxc-parser" dependency (or use
an alternative parser), instead of letting the raw module-not-found propagate to
callers of detectImportsOxc.
---
Nitpick comments:
In `@package.json`:
- Around line 37-39: The peerDependencies entry currently pins "oxc-parser" to
"*", which can introduce breaking API changes at install time; update the
package.json peerDependencies object to use a bounded semver range that matches
the tested catalog (e.g. "oxc-parser": "^0.116.0") instead of "*", and run
install/tests to verify parser: 'oxc' still works; locate the peerDependencies
object in package.json and replace the "*" for "oxc-parser" with the appropriate
caret range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd6e7d9e-a737-4508-a0ab-2530115b9e6d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
package.jsonpnpm-workspace.yamlsrc/detect-acorn.tssrc/detect-estree.tssrc/detect-oxc.tssrc/detect.tssrc/types.tstest/detect-acorn.test.tstest/detect-estree.test.ts
💤 Files with no reviewable changes (1)
- test/detect-acorn.test.ts
Comment on lines
+62
to
+64
| isCJSContext: false, | ||
| firstOccurrence: 0, // TODO: | ||
| } |
There was a problem hiding this comment.
Do not ship placeholder detection metadata.
Returning isCJSContext: false and firstOccurrence: 0 for all ESTree parses breaks behavior parity and can produce incorrect injection decisions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/detect-estree.ts` around lines 62 - 64, The current stub returns constant
metadata (isCJSContext: false, firstOccurrence: 0) which must be replaced with
real detection: inspect the parsed ESTree (AST and tokens) in detect-estree.ts
to determine whether the file uses CommonJS patterns and the index/position of
the first occurrence, then set isCJSContext accordingly and compute
firstOccurrence from the AST/token location (e.g., first
require/module.exports/exports usage or top-level CommonJS node). Update the
code paths that build and return the metadata object so they populate real
values for isCJSContext and firstOccurrence instead of the hardcoded defaults.
Comment on lines
+151
to
+155
| const parameterIdentifiers = parent.params | ||
| .filter(p => p.type === 'Identifier') | ||
| for (const id of parameterIdentifiers) { | ||
| scopeCurrent.declarations.add(id.name) | ||
| } |
There was a problem hiding this comment.
Function params are being declared in the wrong scope.
These params are added before pushScope(node), so they land in the outer scope. That can hide unresolved identifiers outside the function body.
Suggested fix
case 'BlockStatement':
- switch (parent?.type) {
- // for a function body scope, take the function parameters as declarations
- case 'FunctionDeclaration': // e.g. function foo(p1, p2) { ... }
- case 'ArrowFunctionExpression': // e.g. (p1, p2) => { ... }
- case 'FunctionExpression': // e.g. const foo = function(p1, p2) { ... }
- {
- const parameterIdentifiers = parent.params
- .filter(p => p.type === 'Identifier')
- for (const id of parameterIdentifiers) {
- scopeCurrent.declarations.add(id.name)
- }
- break
- }
- }
pushScope(node)
+ switch (parent?.type) {
+ case 'FunctionDeclaration':
+ case 'ArrowFunctionExpression':
+ case 'FunctionExpression':
+ for (const param of parent.params) {
+ if (param.type === 'Identifier')
+ scopeCurrent.declarations.add(param.name)
+ }
+ break
+ }
return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const parameterIdentifiers = parent.params | |
| .filter(p => p.type === 'Identifier') | |
| for (const id of parameterIdentifiers) { | |
| scopeCurrent.declarations.add(id.name) | |
| } | |
| case 'BlockStatement': | |
| pushScope(node) | |
| switch (parent?.type) { | |
| case 'FunctionDeclaration': | |
| case 'ArrowFunctionExpression': | |
| case 'FunctionExpression': | |
| for (const param of parent.params) { | |
| if (param.type === 'Identifier') | |
| scopeCurrent.declarations.add(param.name) | |
| } | |
| break | |
| } | |
| return |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/detect-estree.ts` around lines 151 - 155, The function parameters are
being added to the outer scope because the code iterates parent.params and calls
scopeCurrent.declarations.add(id.name) before creating the function scope; move
the parameter declaration logic to after pushScope(node) (or add them to the
newly created scope object returned/assigned by pushScope) so
parameterIdentifiers (derived from parent.params) are added to the function's
own scope rather than the outer scope; update the block that uses parent.params,
parameterIdentifiers and scopeCurrent.declarations.add to reference the new
scope created by pushScope(node).
Comment on lines
+163
to
+210
| case 'Identifier': | ||
| switch (parent?.type) { | ||
| case 'CallExpression': | ||
| if (parent.callee === node || parent.arguments.includes(node)) | ||
| scopeCurrent.references.add(node.name) | ||
| return | ||
| case 'MemberExpression': | ||
| if (parent.object === node) | ||
| scopeCurrent.references.add(node.name) | ||
| return | ||
| case 'VariableDeclarator': | ||
| if (parent.init === node) | ||
| scopeCurrent.references.add(node.name) | ||
| return | ||
| case 'SpreadElement': | ||
| if (parent.argument === node) | ||
| scopeCurrent.references.add(node.name) | ||
| return | ||
| case 'ClassDeclaration': | ||
| if (parent.superClass === node) | ||
| scopeCurrent.references.add(node.name) | ||
| return | ||
| case 'Property': | ||
| if (parent.value === node) | ||
| scopeCurrent.references.add(node.name) | ||
| return | ||
| case 'TemplateLiteral': | ||
| if (parent.expressions.includes(node)) | ||
| scopeCurrent.references.add(node.name) | ||
| return | ||
| case 'AssignmentExpression': | ||
| if (parent.right === node) | ||
| scopeCurrent.references.add(node.name) | ||
| return | ||
| case 'IfStatement': | ||
| case 'WhileStatement': | ||
| case 'DoWhileStatement': | ||
| if (parent.test === node) | ||
| scopeCurrent.references.add(node.name) | ||
| return | ||
| case 'SwitchStatement': | ||
| if (parent.discriminant === node) | ||
| scopeCurrent.references.add(node.name) | ||
| return | ||
| } | ||
| if (parent?.type.includes('Expression')) | ||
| scopeCurrent.references.add(node.name) | ||
| } |
There was a problem hiding this comment.
Identifier reference detection misses valid statement contexts.
Current parent checks skip identifiers in constructs like return foo, throw foo, and for (...; foo; ...), which can cause missed auto-import matches.
Suggested fix (minimum coverage expansion)
case 'IfStatement':
case 'WhileStatement':
case 'DoWhileStatement':
if (parent.test === node)
scopeCurrent.references.add(node.name)
return
+ case 'ForStatement':
+ if (parent.init === node || parent.test === node || parent.update === node)
+ scopeCurrent.references.add(node.name)
+ return
+ case 'ForInStatement':
+ case 'ForOfStatement':
+ if (parent.right === node)
+ scopeCurrent.references.add(node.name)
+ return
+ case 'ReturnStatement':
+ case 'ThrowStatement':
+ if (parent.argument === node)
+ scopeCurrent.references.add(node.name)
+ return
case 'SwitchStatement':
if (parent.discriminant === node)
scopeCurrent.references.add(node.name)
return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case 'Identifier': | |
| switch (parent?.type) { | |
| case 'CallExpression': | |
| if (parent.callee === node || parent.arguments.includes(node)) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'MemberExpression': | |
| if (parent.object === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'VariableDeclarator': | |
| if (parent.init === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'SpreadElement': | |
| if (parent.argument === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'ClassDeclaration': | |
| if (parent.superClass === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'Property': | |
| if (parent.value === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'TemplateLiteral': | |
| if (parent.expressions.includes(node)) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'AssignmentExpression': | |
| if (parent.right === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'IfStatement': | |
| case 'WhileStatement': | |
| case 'DoWhileStatement': | |
| if (parent.test === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'SwitchStatement': | |
| if (parent.discriminant === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| } | |
| if (parent?.type.includes('Expression')) | |
| scopeCurrent.references.add(node.name) | |
| } | |
| case 'Identifier': | |
| switch (parent?.type) { | |
| case 'CallExpression': | |
| if (parent.callee === node || parent.arguments.includes(node)) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'MemberExpression': | |
| if (parent.object === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'VariableDeclarator': | |
| if (parent.init === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'SpreadElement': | |
| if (parent.argument === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'ClassDeclaration': | |
| if (parent.superClass === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'Property': | |
| if (parent.value === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'TemplateLiteral': | |
| if (parent.expressions.includes(node)) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'AssignmentExpression': | |
| if (parent.right === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'IfStatement': | |
| case 'WhileStatement': | |
| case 'DoWhileStatement': | |
| if (parent.test === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'ForStatement': | |
| if (parent.init === node || parent.test === node || parent.update === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'ForInStatement': | |
| case 'ForOfStatement': | |
| if (parent.right === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'ReturnStatement': | |
| case 'ThrowStatement': | |
| if (parent.argument === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| case 'SwitchStatement': | |
| if (parent.discriminant === node) | |
| scopeCurrent.references.add(node.name) | |
| return | |
| } | |
| if (parent?.type.includes('Expression')) | |
| scopeCurrent.references.add(node.name) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/detect-estree.ts` around lines 163 - 210, The Identifier handling block
misses statement contexts like return, throw, and for-loop tests; update the
switch in the 'Identifier' case to add cases for 'ReturnStatement' and
'ThrowStatement' (check parent.argument === node then call
scopeCurrent.references.add(node.name)) and for 'ForStatement' (check
parent.test === node then add), keeping the existing pattern used for
If/While/DoWhile and Switch; ensure you use the same guard style
(parent.<property> === node) and call scopeCurrent.references.add(node.name) in
each new case.
Comment on lines
+13
to
+14
| case 'oxc': | ||
| return import('./detect-oxc').then(r => r.detectImportsOxc(code, ctx, options)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the file structure
git ls-files src/ | head -20Repository: unjs/unimport
Length of output: 456
🏁 Script executed:
# Check if src/detect.ts exists and examine lines 13-14
if [ -f src/detect.ts ]; then
echo "=== src/detect.ts exists ==="
wc -l src/detect.ts
echo ""
echo "=== Lines around 13-14 ==="
sed -n '1,30p' src/detect.ts | cat -n
fiRepository: unjs/unimport
Length of output: 861
🏁 Script executed:
# Check package.json for oxc-parser dependency status
cat package.json | jq '.peerDependencies, .optionalDependencies, .dependencies' | head -50Repository: unjs/unimport
Length of output: 532
🏁 Script executed:
# Check if src/detect-oxc.ts exists and examine its imports
if [ -f src/detect-oxc.ts ]; then
echo "=== src/detect-oxc.ts first 30 lines ==="
head -30 src/detect-oxc.ts | cat -n
fiRepository: unjs/unimport
Length of output: 432
🏁 Script executed:
# Search for any tests related to oxc-parser missing or error handling
find . -name "*.test.*" -o -name "*.spec.*" | xargs grep -l "oxc" 2>/dev/null | head -5Repository: unjs/unimport
Length of output: 85
🏁 Script executed:
# Check if there's a peerDependenciesMeta field that marks oxc-parser as optional
cat package.json | jq '.peerDependenciesMeta'Repository: unjs/unimport
Length of output: 103
🏁 Script executed:
# Check for any error handling patterns for optional dependencies elsewhere
grep -r "optionalPeer\|peerDependencies\|optional" . --include="*.md" 2>/dev/null | grep -i oxc | head -10Repository: unjs/unimport
Length of output: 39
🏁 Script executed:
# Check the test file to see if there are tests for missing dependencies
cat ./test/detect-estree.test.tsRepository: unjs/unimport
Length of output: 7813
🏁 Script executed:
# Look for any error handling or messaging patterns in the codebase for missing deps
grep -r "requires\|install\|peer dependency" . --include="*.ts" --include="*.js" 2>/dev/null | head -20Repository: unjs/unimport
Length of output: 3605
🏁 Script executed:
# Check if there's a README or docs explaining peer dependencies
head -100 README.md 2>/dev/null || echo "No README found"Repository: unjs/unimport
Length of output: 1932
Add error handling for missing optional oxc-parser dependency.
When parser: 'oxc' is configured and oxc-parser is not installed, the dynamic import of src/detect-oxc.ts rejects with a raw module-not-found error. The static import on line 2 of src/detect-oxc.ts (import { parseSync } from 'oxc-parser') will not fail gracefully. Wrap the import with try-catch to detect this scenario and throw a clear installation message.
Suggested fix
export async function detectImports(
code: string | MagicString,
ctx: UnimportContext,
options?: InjectImportsOptions,
) {
switch (options?.parser) {
case 'acorn':
return import('./detect-acorn').then(r => r.detectImportsAcorn(code, ctx, options))
case 'oxc':
- return import('./detect-oxc').then(r => r.detectImportsOxc(code, ctx, options))
+ try {
+ const r = await import('./detect-oxc')
+ return r.detectImportsOxc(code, ctx, options)
+ }
+ catch (error) {
+ const message = error instanceof Error ? error.message : String(error)
+ if (message.includes('oxc-parser')) {
+ throw new Error('[unimport] parser "oxc" requires optional peer dependency "oxc-parser". Please install it.')
+ }
+ throw error
+ }
default:
return detectImportsRegex(code, ctx, options)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/detect.ts` around lines 13 - 14, When handling case 'oxc' in detect.ts,
wrap the dynamic import(import('./detect-oxc')) in a try/catch so a missing
optional dependency yields a clear error message; specifically, await
import('./detect-oxc') inside an async block and on catch check the error (e.g.,
error.code === 'ERR_MODULE_NOT_FOUND' or error.message includes 'oxc-parser')
and throw a new Error telling the user to install the optional "oxc-parser"
dependency (or use an alternative parser), instead of letting the raw
module-not-found propagate to callers of detectImportsOxc.
antfu
approved these changes
Apr 28, 2026
# Conflicts: # pnpm-lock.yaml # pnpm-workspace.yaml # src/detect-acorn.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/detect-estree.ts (3)
145-160:⚠️ Potential issue | 🟠 MajorFunction params are currently declared in the outer scope.
In this block, params are added before the function-body scope is pushed, so they leak into the parent scope and can hide unresolved identifiers.
Suggested fix
case 'BlockStatement': - switch (parent?.type) { - // for a function body scope, take the function parameters as declarations - case 'FunctionDeclaration': // e.g. function foo(p1, p2) { ... } - case 'ArrowFunctionExpression': // e.g. (p1, p2) => { ... } - case 'FunctionExpression': // e.g. const foo = function(p1, p2) { ... } - { - const parameterIdentifiers = parent.params - .filter(p => p.type === 'Identifier') - for (const id of parameterIdentifiers) { - scopeCurrent.declarations.add(id.name) - } - break - } - } pushScope(node) + switch (parent?.type) { + case 'FunctionDeclaration': + case 'ArrowFunctionExpression': + case 'FunctionExpression': + for (const param of parent.params) { + if (param.type === 'Identifier') + scopeCurrent.declarations.add(param.name) + } + break + } return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/detect-estree.ts` around lines 145 - 160, The function parameter identifiers are being added to scopeCurrent.declarations before the function body scope is created, causing parameters to leak into the outer scope; move the logic that collects and adds parameter identifiers so it runs after pushScope(node) (i.e., create the new function scope first with pushScope in detect-estree, then iterate parent.params for Identifier nodes and add their names to the newly created scopeCurrent.declarations) and ensure this change is applied for the cases 'FunctionDeclaration', 'ArrowFunctionExpression', and 'FunctionExpression' so parameters live in the correct function scope.
62-64:⚠️ Potential issue | 🟠 MajorReplace placeholder ESTree metadata before merge.
Hardcoding
isCJSContextandfirstOccurrencewill misclassify modules and can shift injection behavior. Please derive both from parsed AST usage (e.g., firstrequire(...),module.exports,exports.*occurrence).Proposed fix direction
- return { + const { isCJSContext, firstOccurrence } = detectCjsMetadata(ast) + return { s, strippedCode: code.toString(), matchedImports, - isCJSContext: false, - firstOccurrence: 0, // TODO: + isCJSContext, + firstOccurrence, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/detect-estree.ts` around lines 62 - 64, Replace the hardcoded isCJSContext and firstOccurrence in the object returned by the detect-estree logic by scanning the parsed AST for CommonJS indicators: walk the AST used in this module (the function that currently returns the object containing isCJSContext and firstOccurrence) and detect the earliest node.start of any CallExpression whose callee is an Identifier named "require", any MemberExpression matching "module.exports", and any Identifier usage of "exports" (or "exports.*"); set isCJSContext = true if any of those are found and set firstOccurrence to the smallest start position among those nodes (or leave undefined/null if none), preserving existing shape of the returned metadata.
197-210:⚠️ Potential issue | 🟠 MajorExpand identifier reference handling for statement contexts.
Valid references in
return,throw, andforstatements are still not recorded, which causes missed matches for unresolved identifiers.Suggested coverage expansion
case 'IfStatement': case 'WhileStatement': case 'DoWhileStatement': if (parent.test === node) scopeCurrent.references.add(node.name) return + case 'ForStatement': + if (parent.init === node || parent.test === node || parent.update === node) + scopeCurrent.references.add(node.name) + return + case 'ForInStatement': + case 'ForOfStatement': + if (parent.right === node) + scopeCurrent.references.add(node.name) + return + case 'ReturnStatement': + case 'ThrowStatement': + if (parent.argument === node) + scopeCurrent.references.add(node.name) + return case 'SwitchStatement': if (parent.discriminant === node) scopeCurrent.references.add(node.name) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/detect-estree.ts` around lines 197 - 210, The current identifier handling misses statement contexts like return/throw and for-loops; update the logic in detect-estree.ts (the block using scopeCurrent.references.add(node.name) and parent checks) to also treat ReturnStatement (parent.argument === node) and ThrowStatement (parent.argument === node) as references, and extend ForStatement to check init/test/update (parent.init === node || parent.test === node || parent.update === node) and handle ForInStatement/ForOfStatement by checking left/right (parent.left === node || parent.right === node); keep using scopeCurrent.references.add(node.name) when any of these conditions match so unresolved identifiers in those statement positions are recorded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 37-44: Update the package.json peerDependencies entry for
"oxc-parser" to match the validated dev/prod constraint instead of "*" — change
the peer range for the dependency named "oxc-parser" to the specific semver used
in your devDependency/catalog (e.g. ^0.116.0) so consumers get install-time
warnings for incompatible versions; ensure the corresponding
peerDependenciesMeta optional block remains intact.
---
Duplicate comments:
In `@src/detect-estree.ts`:
- Around line 145-160: The function parameter identifiers are being added to
scopeCurrent.declarations before the function body scope is created, causing
parameters to leak into the outer scope; move the logic that collects and adds
parameter identifiers so it runs after pushScope(node) (i.e., create the new
function scope first with pushScope in detect-estree, then iterate parent.params
for Identifier nodes and add their names to the newly created
scopeCurrent.declarations) and ensure this change is applied for the cases
'FunctionDeclaration', 'ArrowFunctionExpression', and 'FunctionExpression' so
parameters live in the correct function scope.
- Around line 62-64: Replace the hardcoded isCJSContext and firstOccurrence in
the object returned by the detect-estree logic by scanning the parsed AST for
CommonJS indicators: walk the AST used in this module (the function that
currently returns the object containing isCJSContext and firstOccurrence) and
detect the earliest node.start of any CallExpression whose callee is an
Identifier named "require", any MemberExpression matching "module.exports", and
any Identifier usage of "exports" (or "exports.*"); set isCJSContext = true if
any of those are found and set firstOccurrence to the smallest start position
among those nodes (or leave undefined/null if none), preserving existing shape
of the returned metadata.
- Around line 197-210: The current identifier handling misses statement contexts
like return/throw and for-loops; update the logic in detect-estree.ts (the block
using scopeCurrent.references.add(node.name) and parent checks) to also treat
ReturnStatement (parent.argument === node) and ThrowStatement (parent.argument
=== node) as references, and extend ForStatement to check init/test/update
(parent.init === node || parent.test === node || parent.update === node) and
handle ForInStatement/ForOfStatement by checking left/right (parent.left ===
node || parent.right === node); keep using
scopeCurrent.references.add(node.name) when any of these conditions match so
unresolved identifiers in those statement positions are recorded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f911a172-1993-4e82-a968-5cd58b8323e3
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.jsonpnpm-workspace.yamlsrc/detect-estree.ts
✅ Files skipped from review due to trivial changes (1)
- pnpm-workspace.yaml
Comment on lines
+37
to
+44
| "peerDependencies": { | ||
| "oxc-parser": "*" | ||
| }, | ||
| "peerDependenciesMeta": { | ||
| "oxc-parser": { | ||
| "optional": true | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat package.jsonRepository: unjs/unimport
Length of output: 2160
🏁 Script executed:
# Look for pnpm workspace or catalog configuration
fd -t f "pnpm-workspace.yaml|.npmrc|pnpm-lock.yaml" | head -5Repository: unjs/unimport
Length of output: 92
🏁 Script executed:
# Search for oxc-parser usage in the codebase
rg "oxc-parser" --type ts --type js -B 2 -A 2Repository: unjs/unimport
Length of output: 640
🏁 Script executed:
# Check pnpm-workspace.yaml for catalog configuration
cat pnpm-workspace.yamlRepository: unjs/unimport
Length of output: 1045
🏁 Script executed:
# Look at how oxc-parser is imported - check if it's optional/dynamic
cat -n src/detect-oxc.tsRepository: unjs/unimport
Length of output: 392
🏁 Script executed:
# Check what's on/around line 72 to understand "Also applies to: 72-72"
sed -n '70,75p' package.jsonRepository: unjs/unimport
Length of output: 241
Narrow the oxc-parser peer range to a specific constraint.
The peerDependency is currently "*", which accepts any future release and allows breaking upstream changes to be installed without install-time warnings. This surfaced as a runtime error only when the oxc parser path is used. Since the devDependency pins oxc-parser to ^0.116.0 (via the prod catalog), the peer range should match this validated constraint instead of remaining unbounded.
Also applies to: 72-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 37 - 44, Update the package.json peerDependencies
entry for "oxc-parser" to match the validated dev/prod constraint instead of "*"
— change the peer range for the dependency named "oxc-parser" to the specific
semver used in your devDependency/catalog (e.g. ^0.116.0) so consumers get
install-time warnings for incompatible versions; ensure the corresponding
peerDependenciesMeta optional block remains intact.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation