fix(language-service): Support to resolve the re-export component. #62585
Conversation
5ba6e32 to
68a6b85
Compare
|
@atscott Take a look when you have time. |
68a6b85 to
e771902
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the language service to correctly handle re-exported components by resolving the most recent re-export. The changes involve modifying how module specifiers are resolved to include the export name, and using the TypeScript type checker to identify aliased symbols. The implementation looks solid and includes a good test case. I've found a few areas for improvement, mainly around documentation clarity and code duplication, including one high-severity issue with an incorrect example in a comment.
| const sf = program.getSourceFile(tsEntry.tsCompletionEntrySymbolFileName); | ||
| if (sf === undefined) { | ||
| return false; | ||
| } | ||
| const sfSymbol = typeChecker.getSymbolAtLocation(sf); | ||
| if (sfSymbol === undefined) { | ||
| return false; | ||
| } | ||
| const tsEntrySymbol = typeChecker.tryGetMemberInModuleExports( | ||
| tsEntry.tsCompletionEntrySymbolName, | ||
| sfSymbol, | ||
| ); | ||
| if (tsEntrySymbol === undefined) { | ||
| return false; | ||
| } | ||
| const tsEntryType = typeChecker.getTypeOfSymbol(tsEntrySymbol); | ||
| return tsEntryType.symbol?.declarations?.[0] === toImport.node; | ||
| }); | ||
| } |
There was a problem hiding this comment.
This block of code for resolving a symbol from TsCompletionEntryInfo is very similar to the new getSymbolFromTsEntryInfo function in packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts. To improve maintainability and avoid code duplication, consider refactoring this logic into a shared utility function that can be used in both places. If getSymbolFromTsEntryInfo can be exported from checker.ts (or a shared file) and used here, that would be ideal.
There was a problem hiding this comment.
Exporting the getSymbolFromTsEntryInfo function from checker.ts is not the correct approach. Allowing code duplication is acceptable.
There was a problem hiding this comment.
I'll refactor this if there is a better approach.
In the context of TypeScript (TS), a re-exported symbol is considered a distinct symbol. This means that a developer can choose to import either the re-exported symbol or the original symbol. However, in the context of Angular, the re-exported symbol is treated as the same component because it uses the same selector. This pull request will utilize the most recent re-export component file to resolve the module specifier.
e771902 to
2fc2f52
Compare
There was a problem hiding this comment.
Generally LGTM. Unless I'm misunderstanding, I think there might be some risk of always preferring the alias. When working within a package directory, the original definition might be preferable to the alias? That said, if we're only ever allowing one completion item, then we have to make a choice and likely the alias is the one we want most of the time?
Yes, personally, I think the alias is the best one. Now the ts tends to choose the shortest path. 
If a package re-exports a component, it means importing that component from the package is allowed by the package. The package can provide better options by adjusting the exported symbols(for example: don't re-export it). Maybe in the future, Angular LS will support display choice for the same component. This can be removed. It's better to have a rule here to allow the developer to adjust the code. |
In the context of TypeScript (TS), a re-exported symbol is considered a distinct symbol.
This means that a developer can choose to import either the re-exported symbol or
the original symbol. However, in the context of Angular, the re-exported symbol
is treated as the same component because it uses the same selector.
This pull request will utilize the most recent re-export component file to
resolve the module specifier.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information