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
feat(language-service): add support for text replacement #33091
Conversation
This is WIP. I believe #31897 will need to land before this really works, because we are facing issues relating to invalid template AST sourceSpans (similar to what we saw, and still see, with the indexer module). In particular, I've included a test that fails with a @Component({
template: `<a href="https://angular.io" (click)="han"></a>`,
})
export class AngularLinkComponent {
handleClick() {}
} Generating completions for |
@andrius-pra this may interest you as well. |
Possible fix: You can add special case when node is ASTWithSource. let closestAst !: AstPath<AST>;
if (ast instanceof ASTWithSource) {
closestAst = findAstAt(ast.ast, templatePosition - ast.sourceSpan.start);
} else {
closestAst = findAstAt(ast, templatePosition - templateAstTail.sourceSpan.start.offset);
} |
Good point. I'll try this out, but I think I did something similar in June and it wasn't very reliable. Thank you! |
0523e56
to
a3268cc
Compare
Prior to this commit, the absolute spans (relative to template source file rather than the start of an expression) of expressions in a template attribute like `*ngIf` were generated incorrectly, equating to the relative spans. This fixes the bug by passing an `absoluteOffset` parameter when parsing template bindings. Through some levels of indirection, this is required for the Language Service to support text replacement in angular#33091.
Prior to this commit, the absolute spans (relative to template source file rather than the start of an expression) of expressions in a template attribute like `*ngIf` were generated incorrectly, equating to the relative spans. This fixes the bug by passing an `absoluteOffset` parameter when parsing template bindings. Through some levels of indirection, this is required for the Language Service to support text replacement in angular#33091. (cherry picked from commit 9e2b784)
#33189) Prior to this commit, the absolute spans (relative to template source file rather than the start of an expression) of expressions in a template attribute like `*ngIf` were generated incorrectly, equating to the relative spans. This fixes the bug by passing an `absoluteOffset` parameter when parsing template bindings. Through some levels of indirection, this is required for the Language Service to support text replacement in #33091. PR Close #33189
angular#33189) Prior to this commit, the absolute spans (relative to template source file rather than the start of an expression) of expressions in a template attribute like `*ngIf` were generated incorrectly, equating to the relative spans. This fixes the bug by passing an `absoluteOffset` parameter when parsing template bindings. Through some levels of indirection, this is required for the Language Service to support text replacement in angular#33091. PR Close angular#33189
angular#33189) Prior to this commit, the absolute spans (relative to template source file rather than the start of an expression) of expressions in a template attribute like `*ngIf` were generated incorrectly, equating to the relative spans. This fixes the bug by passing an `absoluteOffset` parameter when parsing template bindings. Through some levels of indirection, this is required for the Language Service to support text replacement in angular#33091. PR Close angular#33189
Moves to using the absolute span of an expression AST (relative to an entire template) rather than a relative span (relative to the start of the expression) to find an expression AST given a position in a template. This is part of the changes needed to support text replacement in templates (angular#33091).
…#33387) Moves to using the absolute span of an expression AST (relative to an entire template) rather than a relative span (relative to the start of the expression) to find an expression AST given a position in a template. This is part of the changes needed to support text replacement in templates (#33091). PR Close #33387
a3268cc
to
7861004
Compare
Okay, so half the problem seems to be solved -- we can now replace spans when performing a completion at the end of the text span! So in a case like Unfortunately, we cannot currently replace spans when selecting a completion in the middle of the span. For example, say we had already typed
We'd expect that selecting the completion One way to resolve this is to go work on the Angular AST even more, and have completely atomic ASTs for every semantic word or symbol in an Angular program. This would be a major overhaul and would take a lot of time, as we would have to do it for both expression ASTs and markup (HTML) ASTs. I think this is the most correct solution, but maybe not the most practical. The other option is to perform some kind of matching algorithm - maybe we can figure out where the span to replace starts but looking at the overlap with the completion, then erasing the entire word in the span starting from the determined start location. I think for now this would work fine, but there is at least the issue that would not work with fuzzy-matched completions. What do you think, @kyliau and @andrius-pra? |
I think it's a "good-enough" solution with the absolute span. In edge cases like you mentioned, we could just set the replacement span to the nearest word boundary. |
…angular#33387) Moves to using the absolute span of an expression AST (relative to an entire template) rather than a relative span (relative to the start of the expression) to find an expression AST given a position in a template. This is part of the changes needed to support text replacement in templates (angular#33091). PR Close angular#33387
Okay, maybe it's even safer to implement expanding the word until a word boundary rather than relying on a span. Let me see how that does. |
Adds a `replacementSpan` field on a completion that will allow typed text to be replaced with the suggested completion value if a user selects the completion. Previously, the completion value would simply be appended to the text already typed. E.g. if we had ``` {{ti}} ``` typed in a template and `title` was recommended as a completion and selected, the template would become ``` {{tititle}} ``` With `replacementSpan`, the original text `ti` will be replaced for `title`.
bcd8c7f
to
fd03f86
Compare
Please also rebase this PR, thank you! |
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.
LGTM, thank you!
Adds a `replacementSpan` field on a completion that will allow typed text to be replaced with the suggested completion value if a user selects the completion. Previously, the completion value would simply be appended to the text already typed. E.g. if we had ``` {{ti}} ``` typed in a template and `title` was recommended as a completion and selected, the template would become ``` {{tititle}} ``` With `replacementSpan`, the original text `ti` will be replaced for `title`. PR Close angular#33091
Adds a `replacementSpan` field on a completion that will allow typed text to be replaced with the suggested completion value if a user selects the completion. Previously, the completion value would simply be appended to the text already typed. E.g. if we had ``` {{ti}} ``` typed in a template and `title` was recommended as a completion and selected, the template would become ``` {{tititle}} ``` With `replacementSpan`, the original text `ti` will be replaced for `title`. PR Close angular#33091
Adds a `replacementSpan` field on a completion that will allow typed text to be replaced with the suggested completion value if a user selects the completion. Previously, the completion value would simply be appended to the text already typed. E.g. if we had ``` {{ti}} ``` typed in a template and `title` was recommended as a completion and selected, the template would become ``` {{tititle}} ``` With `replacementSpan`, the original text `ti` will be replaced for `title`. PR Close angular#33091
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Adds a
replacementSpan
field on a completion that will allow typedtext to be replaced with the suggested completion value if a user
selects the completion. Previously, the completion value would simply be
appended to the text already typed. E.g. if we had
typed in a template and
title
was recommended as a completion andselected, the template would become
With
replacementSpan
, the original textti
will be replaced fortitle
.PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?