feat(typescript-plugin): support go to definition for component props#96
feat(typescript-plugin): support go to definition for component props#96iamzjt-front-end wants to merge 1 commit intompx-ecology:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds comprehensive Go-to-Definition test fixtures and extends the TypeScript plugin with template-aware resolution: it extracts template attribute/tag names, resolves child component paths from usingComponents, and locates prop definitions across component boundaries via AST analysis. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Editor as Editor/LSP
participant Plugin as TS Plugin
participant Template as Template Parser
participant Resolver as Component Resolver
participant AST as AST Analyzer
User->>Editor: Invoke "Go to Definition" at template position
Editor->>Plugin: getDefinitionAndBoundSpan(file, offset)
Plugin->>Template: extractAttributeNameAtPosition(offset)
Template-->>Plugin: attribute name & isEvent flag
alt non-event attribute
Plugin->>Template: extractTagNameAtPosition(offset)
Template-->>Plugin: tag name
Plugin->>Resolver: findComponentPath(tag name, sfc)
Resolver-->>Plugin: candidate component path(s)
Plugin->>Resolver: resolveComponentPath(parentFile, candidatePath)
Resolver-->>Plugin: resolved child component file
Plugin->>AST: tryFindComponentPropDefinitionByPath(resolvedPath, propName)
AST->>AST: search defineProps / withDefaults / createComponent / type aliases
AST-->>Plugin: DefinitionInfo (if found)
Plugin->>Editor: Return child prop definition location
else event attribute
Plugin->>Editor: Fall back to original definitions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/typescript-plugin/src/common.ts`:
- Around line 319-321: The current check in common.ts uses "if (isMpxFile &&
attrInfo && !attrInfo.isEvent) continue" which can skip valid targets when
tagInfo is undefined; change the condition to require tagInfo as well (e.g.,
include tagInfo truthiness so it becomes something like "if (isMpxFile &&
tagInfo && attrInfo && !attrInfo.isEvent) continue") so attributes are only
skipped when they are inside a tag; update the if-statement referencing
isMpxFile, tagInfo, and attrInfo accordingly.
🧹 Nitpick comments (4)
packages/typescript-plugin/src/common.ts (3)
205-213: Remove or conditionally enable debug logging for production.Multiple
console.logstatements throughout this implementation will pollute output in production. Consider removing them or using a configurable debug flag.♻️ Suggested approach
+const DEBUG = process.env.MPX_DEBUG === 'true' + +function debugLog(...args: any[]) { + if (DEBUG) { + console.log('[Mpx Go to Definition]', ...args) + } +} + return (fileName, position) => { - console.log('[Mpx Go to Definition] getDefinitionAndBoundSpan called:', { - fileName, - position, - }) + debugLog('getDefinitionAndBoundSpan called:', { fileName, position })Apply similar changes to all other
console.logcalls in lines 212, 261-266, 276, 287-290, 305-316, 431-436, 514, 527, 582-588, 593-596, 602, 610-613, 616-624, 633-636, and 642-648.
858-887: Type alias lookup is limited toTypeLiteralNode.The function only resolves type aliases defined as object literals (
type Props = { ... }). It doesn't handle:
- Interface declarations (
interface Props { ... })- Extended/intersection types
- Imported types
This is acceptable for the current scope, but worth noting for future enhancement if users define props using interfaces.
560-566: Hardcoded.mpxextension does not respectmpxOptions.extensions.The
resolveComponentPathfunction always appends.mpxregardless of configured extensions. WhilempxOptions.extensionsdefaults to['.mpx'], the code should respect custom extensions if configured. The function lacks access tompxOptionsand would need it passed through from the calling context.To fix this, both
findComponentPathandresolveComponentPathneed access tompxOptions. ForresolveComponentPath, check if the path already has a configured extension before appending:function resolveComponentPath( componentPath: string, currentFileName: string, + mpxOptions: MpxCompilerOptions, ): string { if (componentPath.startsWith('./') || componentPath.startsWith('../')) { // ... path resolution logic ... let resolvedPath = currentParts.join('/') - if (!resolvedPath.endsWith('.mpx')) { - resolvedPath += '.mpx' + const hasConfiguredExtension = mpxOptions.extensions.some(ext => + resolvedPath.endsWith(ext) + ) + if (!hasConfiguredExtension) { + resolvedPath += mpxOptions.extensions[0] || '.mpx' } return resolvedPath } return componentPath }inspect-extension/ts-features/ts-go-to-definition/parent-component-setup.mpx (1)
49-55: Consider adding a comment clarifying non-reactive data is intentional for this test fixture.The data variables are defined as plain
constrather than usingref()from the Composition API. While this works for the "Go to Definition" test scenarios, adding a brief comment would clarify that reactivity is intentionally omitted for simplicity.💡 Suggested clarification
<script setup lang="ts"> -// 响应式数据 +// 测试数据（此处省略 ref() 以简化测试用例，仅用于跳转定义测试） const title = 'Hello' const count = 10
iamzjt-front-end
force-pushed
the
main
branch
from
717897e to
31f2633
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/typescript-plugin/src/common.ts`:
- Around line 721-738: The defineProps handling currently only handles inline
type literals; add handling for type reference patterns by detecting when
node.typeArguments[0] is a TypeReference (ts.isTypeReferenceNode) and then call
findTypeAliasAndPropAst(ts, scriptAst, typeArg, possibleNames) similar to the
withDefaults path; if that returns a non-null result use it, otherwise fall back
to the existing findPropInTypeLiteralAst logic for ts.isTypeLiteralNode — update
the block around node.typeArguments, typeArg, and result accordingly so
defineProps resolves type aliases like defineProps<Props>().
- Around line 397-438: extractAttributeNameAtPosition currently omits '@' when
scanning and doesn't recognize/v-normalize Vue event/bind prefixes, and it may
capture tokens inside attribute values; update the backward and forward scan
regexes to include '@', extend the event-detection regex to match '@' and
'v-on:' as well as existing bind/catch variants, and normalize binding prefixes
like 'v-bind:' and ':' to the canonical name before returning (operate on
attrName). Also add a guard that checks surrounding characters for '=' (or
presence of '=' between name end and next quote) to avoid matching inside
attribute values (e.g., title="@click"), and keep debug logging of
attrName/isEvent/start/end for verification.
8a31c50 to
0c5de00
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/typescript-plugin/src/common.ts`:
- Around line 868-887: The function findTypeAliasAndPropAst only checks
ts.isTypeAliasDeclaration; extend its visit logic to also handle
ts.isInterfaceDeclaration(node) with node.name.text === typeName and then
iterate node.members exactly like findPropInTypeLiteralAst does for
TypeLiteralNodes to locate a property matching possibleNames; set result to the
same {start,length} shape when a matching property/member is found. In short: in
visit add an else-if branch for ts.isInterfaceDeclaration, use node.members to
find the prop (reusing the same matching logic as findPropInTypeLiteralAst) and
assign result so interface-based props resolve the same way as type aliases.
- Around line 845-861: The function findPropInTypeLiteralAst only checks
ts.isIdentifier(name) and misses string-literal property names; update it to
also detect ts.isStringLiteral(name) (as findPropInObjectLiteralAst does),
compare name.text against possibleNames (to match kebab-case like
"show-header"), and return the same start/length using name.getStart(sourceFile)
and name.getEnd() so string-literal keys are correctly found.
🧹 Nitpick comments (1)
packages/typescript-plugin/src/common.ts (1)
205-316: Gate verbose go-to-definition logging behind a debug flag.These
console.logcalls sit on a hot LS path and can spam output / degrade responsiveness. Consider a debug toggle or logger-level guard.
iamzjt-front-end
force-pushed
the
main
branch
from
0c5de00 to
3cd642e
Compare
- Add support for jumping from component attributes to child component's properties definition
- Support Options API (createComponent/createPage/Component/Page)
- Support Composition API (defineProps with generics and object syntax)
- Support withDefaults(defineProps<Props>(), {...}) syntax
- Support kebab-case to camelCase conversion (e.g., show-header → showHeader)
iamzjt-front-end
force-pushed
the
main
branch
from
3cd642e to
70141f3
Compare
|
@wangshunnn 已修复并 resolve 所有评论，CI 通过了，麻烦 approve 一下🙏 |
|
整体上实现有些违背插件核心“利用虚拟文件借助ts能力进行跳转、类型提示等能力”，同时目前项目里在核心core部分已经解析了components。应该可以直接拿到使用的。可以再看一下相关实现，优化一下成走虚拟文件，发挥变量之间关系来达到跳转。 @iamzjt-front-end |
2 participants
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.