feat(language-service): Add getTypeDefinitionAtPosition (go to type d… #39145

atscott · 2020-10-06T22:43:35Z

…efinition)

This commit adds the implementation for providing "go to type definition"
functionality in the Ivy Language Service.

packages/language-service/ivy/definitions.ts

ayazhafiz

This is so exciting

packages/language-service/ivy/definitions.ts

packages/language-service/ivy/test/type_definitions_spec.ts

ayazhafiz · 2020-10-06T23:23:26Z

packages/language-service/ivy/test/type_definitions_spec.ts

+      it('should return nothing for input providers', () => {
+        const definitions = getTypeDefinitionsAndAssertBoundSpan({
+          templateOverride: `<test-comp [tcN¦ame]="name"></test-comp>`,
+        });
+        expect(definitions).toEqual([]);


Wouldn't tcName have a type, namely string?

This is actually just the behavior of the ts language service itself for primitives. However, if I set the type to String rather than string, I do get definitions from the lib.es2015.core.d.ts, lib.es5.d.ts, etc. I think this case is generally covered in the new test that Keen requested (go to type definition of toLowerCase)

Gotcha. I think it would be worth reworking this test to use a value that has a go-to-definition type, because I think the test title "should return nothing for input providers" is incorrect - in general we do want to return something for input providers, when there is something to return.

packages/language-service/ivy/test/type_definitions_spec.ts

kyliau

minor comments. LGTM overall

packages/language-service/ivy/definitions.ts

kyliau · 2020-10-07T00:57:46Z

packages/language-service/ivy/definitions.ts

  }
 }
+
+function getDefinitionsForSymbols(
+    symbols: {shimLocation: ShimLocation}[],


Instead of the custom type, why not just pass in the shim locations?

It makes calling the function easier. I would otherwise have to map all the symbols before passing them in. I also can't use the Symbol type because some don't have shimLocation at the top level. If you feel strongly about this, I can change it.

packages/language-service/ivy/definitions.ts

packages/language-service/ivy/test/type_definitions_spec.ts

atscott

PTAL. I had to make some adjustments to the getDirectiveMatches to allow matching compound selectors that included the tag name.

atscott · 2020-10-07T16:14:31Z

packages/language-service/ivy/definitions.ts

  }
 }
+
+function getDefinitionsForSymbols(
+    symbols: {shimLocation: ShimLocation}[],


It makes calling the function easier. I would otherwise have to map all the symbols before passing them in. I also can't use the Symbol type because some don't have shimLocation at the top level. If you feel strongly about this, I can change it.

packages/language-service/ivy/definitions.ts

atscott · 2020-10-07T17:16:57Z

packages/language-service/ivy/test/type_definitions_spec.ts

+      it('should return nothing for input providers', () => {
+        const definitions = getTypeDefinitionsAndAssertBoundSpan({
+          templateOverride: `<test-comp [tcN¦ame]="name"></test-comp>`,
+        });
+        expect(definitions).toEqual([]);


This is actually just the behavior of the ts language service itself for primitives. However, if I set the type to String rather than string, I do get definitions from the lib.es2015.core.d.ts, lib.es5.d.ts, etc. I think this case is generally covered in the new test that Keen requested (go to type definition of toLowerCase)

packages/language-service/ivy/test/type_definitions_spec.ts

ayazhafiz · 2020-10-07T19:55:27Z

packages/language-service/ivy/definitions.ts

+    return symbols.reduce((result, {shimLocation}) => {
+      const defs = this.tsLS.getDefinitionAtPosition(
+                       shimLocation.shimPath, shimLocation.positionInShimFile) ??
+          [];


nit: clang format is really weird here, maybe something like

Suggested change

return symbols.reduce((result, {shimLocation}) => {

const defs = this.tsLS.getDefinitionAtPosition(

shimLocation.shimPath, shimLocation.positionInShimFile) ??

[];

return symbols.reduce((result, {shimLocation}) => {

const {shimPath, positionInShimFile} = shimLocation;

const defs = this.tsLS.getDefinitionAtPosition(shimPath, positionInShimFile) ?? [];

to make it a bit more linear?

Also, can we use map and flatten here rather than creating a new array on each reduce iteration?

ayazhafiz · 2020-10-07T20:00:16Z

packages/language-service/ivy/definitions.ts

+    switch (symbol.kind) {
+      case SymbolKind.Template: {
+        const matches = getDirectiveMatches(
+            {name: symbol.templateNode.tagName, kind: DirectiveMatchKind.TAG_NAME},


can we pass the name and kind as separate params? Constructing an object feels verbose.

Refactored this into two totally separate functions. This was feeling a little bit like a code smell the way it was.

ayazhafiz · 2020-10-07T20:06:35Z

packages/language-service/ivy/definitions.ts

+    }, [] as ts.DefinitionInfo[]);
+  }
+
+  private getSymbolAndNodeAtPosition(fileName: string, position: number): DefinitionMeta {


The usages of this function return value unpack symbol and node and check that both are defined before moving on, so I think it would be better to make the return type DefinitionMeta|undefined. I think this would make it clearer that the metadata is either entirely defined with a symbol and node, or not at all.

Done. I have a WIP branch that distinguish between templateInfo is undefined vs if the node/symbol lookup failed so I just extracted the templateInfo getter out of this function.

packages/language-service/ivy/test/type_definitions_spec.ts

packages/language-service/ivy/utils.ts

ayazhafiz

nice 👍

packages/language-service/ivy/definitions.ts

packages/language-service/ivy/test/type_definitions_spec.ts

packages/language-service/ivy/utils.ts

…efinition) This commit adds the implementation for providing "go to type definition" functionality in the Ivy Language Service.

angular-automatic-lock-bot · 2020-11-09T16:39:02Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

atscott added action: review The PR is still awaiting reviews from at least one requested reviewer area: language-service Issues related to Angular's VS Code language service target: major This PR is targeted for the next major release labels Oct 6, 2020

atscott requested review from kyliau and ayazhafiz October 6, 2020 22:43

atscott self-assigned this Oct 6, 2020

ngbot bot added this to the needsTriage milestone Oct 6, 2020

google-cla bot added the cla: yes label Oct 6, 2020

ayazhafiz reviewed Oct 6, 2020

View reviewed changes

packages/language-service/ivy/definitions.ts Outdated Show resolved Hide resolved

ayazhafiz reviewed Oct 6, 2020

View reviewed changes

kyliau approved these changes Oct 7, 2020

View reviewed changes

ChellappanRajan mentioned this pull request Oct 7, 2020

docs: fix for HttpRequest expansion panel #39166

Closed

12 tasks

atscott force-pushed the gototypedefinition branch 3 times, most recently from ecdbf2d to d43ea0f Compare October 7, 2020 18:52

atscott commented Oct 7, 2020

View reviewed changes

atscott requested review from ayazhafiz and kyliau October 7, 2020 18:56

atscott force-pushed the gototypedefinition branch 3 times, most recently from f0ecc84 to 61d11e5 Compare October 7, 2020 20:08

ayazhafiz reviewed Oct 7, 2020

View reviewed changes

atscott force-pushed the gototypedefinition branch from 61d11e5 to 4baab21 Compare October 8, 2020 17:08

atscott requested a review from ayazhafiz October 8, 2020 19:07

ayazhafiz approved these changes Oct 9, 2020

View reviewed changes

packages/language-service/ivy/definitions.ts Outdated Show resolved Hide resolved

atscott force-pushed the gototypedefinition branch from 4baab21 to 29f9779 Compare October 9, 2020 15:35

kyliau approved these changes Oct 9, 2020

View reviewed changes

packages/language-service/ivy/test/type_definitions_spec.ts Show resolved Hide resolved

packages/language-service/ivy/utils.ts Outdated Show resolved Hide resolved

packages/language-service/ivy/utils.ts Show resolved Hide resolved

packages/language-service/ivy/utils.ts Show resolved Hide resolved

atscott force-pushed the gototypedefinition branch from 29f9779 to 0ffc386 Compare October 9, 2020 16:59

atscott removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 9, 2020

atscott added the action: merge The PR is ready for merge by the caretaker label Oct 9, 2020

feat(language-service): Add getTypeDefinitionAtPosition (go to type d…

25a4d4f

…efinition) This commit adds the implementation for providing "go to type definition" functionality in the Ivy Language Service.

atscott force-pushed the gototypedefinition branch from 0ffc386 to 25a4d4f Compare October 9, 2020 17:11

atscott closed this in a84976f Oct 9, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 9, 2020

feat(language-service): Add getTypeDefinitionAtPosition (go to type d… #39145

feat(language-service): Add getTypeDefinitionAtPosition (go to type d… #39145

Uh oh!

Conversation

atscott commented Oct 6, 2020

Uh oh!

Uh oh!

ayazhafiz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kyliau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ayazhafiz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!