feat(language-service): provide hover for microsyntax in structural directive #34847
Conversation
ivanwonder
force-pushed
the
provide-hover-for-microsyntax
branch
2 times, most recently
from
053acb9
to
0d749ab
Compare
ivanwonder
force-pushed
the
provide-hover-for-microsyntax
branch
from
0d749ab
to
2ceb8f4
Compare
petebacondarwin
added
area: language-service
ivanwonder
force-pushed
the
provide-hover-for-microsyntax
branch
from
2ceb8f4
to
e1830c6
Compare
ivanwonder
force-pushed
the
provide-hover-for-microsyntax
branch
from
e1830c6
to
6325b98
Compare
ivanwonder
force-pushed
the
provide-hover-for-microsyntax
branch
from
6325b98
to
70b4aae
Compare
ivanwonder
force-pushed
the
provide-hover-for-microsyntax
branch
2 times, most recently
from
1ae65e3
to
38587ec
Compare
ivanwonder
force-pushed
the
provide-hover-for-microsyntax
branch
2 times, most recently
from
a624fde
to
8796adb
Compare
|
The |
|
@ivanwonder I think for now, the easiest and most correct thing to do for this PR will be to overwrite the symbol found for the structural directive in |
|
@ayazhafiz So, I also need to make the assertion work for the length of |
|
Yes. I do not think that is a bug. |
ivanwonder
force-pushed
the
provide-hover-for-microsyntax
branch
from
2fe326e
to
5ac0787
Compare
…ral directive For the structural directive, the 'path' will contain multiple `BoundDirectivePropertyAst` which depends on the number of directive property in the attribute value(e.g. '*ngFor="let item of []; trackBy: test;"', it has 2 `BoundDirectivePropertyAst`, 'ngForOf' and 'ngForTrackBy').
| // For the structural directive, only care about the last template AST. | ||
| if (attribute?.name.startsWith('*')) { | ||
| toVisit.splice(0, toVisit.length - 1); | ||
| } |
There was a problem hiding this comment.
For the structural directive, the 'path' will contain multiple BoundDirectivePropertyAst which depends on the number of directive property in the attribute value(e.g. '*ngFor="let item of []; trackBy: test;"', it has 2 BoundDirectivePropertyAst, 'ngForOf' and 'ngForTrackBy').
There was a problem hiding this comment.
@ayazhafiz I think this is why the length of definitions has doubled.
There was a problem hiding this comment.
You're right, this is because the directive property AST spans the entire attribute value. So the binding is parsed twice, and we end up with two pairs of each definition. I will open a PR to fix this by pruning duplicate values, but feel free to go ahead and just validate two of them for now; again, this PR should not be blocked by the issue (most language service protocol implementations will ignore the duplicate values anyway).
There was a problem hiding this comment.
You can review my PR again. If it’s ok, can you approve it?
I test it in vscode, the number of definitions show in the tooltip doesn’t ignore the duplication.
There was a problem hiding this comment.
When you click it, it will show only one definition.
There was a problem hiding this comment.
Maybe we should filter the definitions here.
angular/packages/language-service/src/definitions.ts
Lines 68 to 71 in 3d4b143
There was a problem hiding this comment.
Yes, we will filter the definitions. The PR looks good to me. I think for now this is fine, will fixup after the definition-pruning PR.
There was a problem hiding this comment.
There is a uniqueBySpan function in diagnostics.ts that could be useful here.
ayazhafiz
added
action: merge
Sometimes, a request for definitions will return multiple of the same
definition. This can happen in at least the cases of
- two-way bindings (one of the same definition for the property and
event binding)
- multiple template binding expressions in the same attribute
- something like "*ngFor="let i of items; trackBy: test" has two
template bindings, resulting in two template binding ASTs at the
same location (the attribute span). The language service then parses
both of these bindings individually, resulting in two independent
but identical definitions. For more context, see angular#34847 (comment).
This commit prunes duplicate definitions by signing definitions with
their location, and checking if that location signature has been seen in
a previous definition returned to the client.
Sometimes, a request for definitions will return multiple of the same
definition. This can happen in at least the cases of
- two-way bindings (one of the same definition for the property and
event binding)
- multiple template binding expressions in the same attribute
- something like "*ngFor="let i of items; trackBy: test" has two
template bindings, resulting in two template binding ASTs at the
same location (the attribute span). The language service then parses
both of these bindings individually, resulting in two independent
but identical definitions. For more context, see angular#34847 (comment).
This commit prunes duplicate definitions by signing definitions with
their location, and checking if that location signature has been seen in
a previous definition returned to the client.
…ral directive (#34847) For the structural directive, the 'path' will contain multiple `BoundDirectivePropertyAst` which depends on the number of directive property in the attribute value(e.g. '*ngFor="let item of []; trackBy: test;"', it has 2 `BoundDirectivePropertyAst`, 'ngForOf' and 'ngForTrackBy'). PR Close #34847
…ral directive (#34847) For the structural directive, the 'path' will contain multiple `BoundDirectivePropertyAst` which depends on the number of directive property in the attribute value(e.g. '*ngFor="let item of []; trackBy: test;"', it has 2 `BoundDirectivePropertyAst`, 'ngForOf' and 'ngForTrackBy'). PR Close #34847
Sometimes, a request for definitions will return multiple of the same
definition. This can happen in at least the cases of
- two-way bindings (one of the same definition for the property and
event binding)
- multiple template binding expressions in the same attribute
- something like "*ngFor="let i of items; trackBy: test" has two
template bindings, resulting in two template binding ASTs at the
same location (the attribute span). The language service then parses
both of these bindings individually, resulting in two independent
but identical definitions. For more context, see angular#34847 (comment).
This commit prunes duplicate definitions by signing definitions with
their location, and checking if that location signature has been seen in
a previous definition returned to the client.
Sometimes, a request for definitions will return multiple of the same
definition. This can happen in at least the cases of
- two-way bindings (one of the same definition for the property and
event binding)
- multiple template binding expressions in the same attribute
- something like "*ngFor="let i of items; trackBy: test" has two
template bindings, resulting in two template binding ASTs at the
same location (the attribute span). The language service then parses
both of these bindings individually, resulting in two independent
but identical definitions. For more context, see #34847 (comment).
This commit prunes duplicate definitions by signing definitions with
their location, and checking if that location signature has been seen in
a previous definition returned to the client.
PR Close #34995
Sometimes, a request for definitions will return multiple of the same
definition. This can happen in at least the cases of
- two-way bindings (one of the same definition for the property and
event binding)
- multiple template binding expressions in the same attribute
- something like "*ngFor="let i of items; trackBy: test" has two
template bindings, resulting in two template binding ASTs at the
same location (the attribute span). The language service then parses
both of these bindings individually, resulting in two independent
but identical definitions. For more context, see #34847 (comment).
This commit prunes duplicate definitions by signing definitions with
their location, and checking if that location signature has been seen in
a previous definition returned to the client.
PR Close #34995
…ral directive (angular#34847) For the structural directive, the 'path' will contain multiple `BoundDirectivePropertyAst` which depends on the number of directive property in the attribute value(e.g. '*ngFor="let item of []; trackBy: test;"', it has 2 `BoundDirectivePropertyAst`, 'ngForOf' and 'ngForTrackBy'). PR Close angular#34847
…34995) Sometimes, a request for definitions will return multiple of the same definition. This can happen in at least the cases of - two-way bindings (one of the same definition for the property and event binding) - multiple template binding expressions in the same attribute - something like "*ngFor="let i of items; trackBy: test" has two template bindings, resulting in two template binding ASTs at the same location (the attribute span). The language service then parses both of these bindings individually, resulting in two independent but identical definitions. For more context, see angular#34847 (comment). This commit prunes duplicate definitions by signing definitions with their location, and checking if that location signature has been seen in a previous definition returned to the client. PR Close angular#34995
|
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. |
In #34564, I only support hover over the directive name. And have a problem, when hovering over
*ngFor="exp", it will showngForOfproperty ofNgForOfclass, this pr will show(directive) CommonModule.NgForOf: class. This PR also supports the microsyntax(e.g.trackBy,of).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