feat(language-service): Allow auto-imports of a pipe via quick fix when its selector is used. by dylhunn · Pull Request #48354 · angular/angular

dylhunn · 2022-12-05T01:37:16Z

Four commits, as below:

refactor(language-service): Improve the quick fix auto-import tests.

Currently, the auto-import tests are very difficult to debug, because the expectations are buried several function calls deep, and also because the multi-line replacements are hard to deal with. The auto-import tests now use a different approach, which can be easily debugged from the failure message. In addition, several new tests have been added to cover more cases.

refactor(compiler-cli): Make `getKnown` return an array of nodes.

MetadataReaderWithIndex.getKnown currently returns an iterator. It will be easier to work with for upcoming usages if it returns an array instead.

feat(language-service): Introduce a new NgModuleIndex, and use it to suggest re-exports.

NgModules can re-export other NgModules, which transitively includes all traits exported by the re-exported NgModule. We want to be able to suggest all re-exports of a component when trying to auto-import it.

Previously, we used an approximation when importing from NgModules: we looked at a Component's metadata, and just imported the declaring NgModule. However, this is not technically correct -- the declaring NgModule it is not necessarily the same one that exports it for the current scope. (Indeed, there could be multiple re-exports!) As a replacement, I have implemented a more general solution.

This PR introduces a new class on the template type checker, called NgModuleIndex. When queried, it conducts a depth-first-search over the NgModule import/export graph, in order to find all NgModules anywhere in the current dependency graph, as well as all exports of those NgModules. This allows the language service to suggest all of the re-exports, in addition to the original export.

feat(language-service): Allow auto-imports of a pipe via quick fix when its selector is used, both directly and via reexports.

A previous PR introduced a new compiler abstraction that tracks all known exports and re-exports of Angular traits. This PR harnesses that abstraction in the language service, in order to allow automatic imports of pipes.

dylhunn · 2022-12-07T20:26:55Z

@atscott Just want to keep this on your radar -- no rush though.

atscott

reviewed-for: fw-language-service

packages/language-service/src/codefixes/fix_missing_import.ts

atscott

reviewed-for: language-service

dylhunn · 2023-01-09T23:45:11Z

Back to @alxhub with a new design for NgModuleIndex -- we should be in the home stretch now!

Currently, the auto-import tests are very difficult to debug, because the `expect`ations are buried several function calls deep, and also because the multi-line replacements are hard to deal with. The auto-import tests now use a different approach, which can be easily debugged from the failure message. In addition, several new tests have been added to cover more cases.

`MetadataReaderWithIndex.getKnown` currently returns an iterator. It will be easier to work with for upcoming usages if it returns an array instead.

packages/compiler-cli/src/ngtsc/metadata/src/ng_module_index.ts

packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts

packages/compiler-cli/src/ngtsc/metadata/src/util.ts

…suggest re-exports. NgModules can re-export other NgModules, which transitively includes all traits exported by the re-exported NgModule. We want to be able to suggest *all* re-exports of a component when trying to auto-import it. Previously, we used an approximation when importing from NgModules: we looked at a Component's metadata, and just imported the declaring NgModule. However, this is not technically correct -- the declaring NgModule it is not necessarily the same one that exports it for the current scope. (Indeed, there could be multiple re-exports!) As a replacement, I have implemented a more general solution. This PR introduces a new class on the template type checker, called `NgModuleIndex`. When queried, it conducts a depth-first-search over the NgModule import/export graph, in order to find all NgModules anywhere in the current dependency graph, as well as all exports of those NgModules. This allows the language service to suggest all of the re-exports, in addition to the original export.

…en its selector is used, both directly and via reexports. A previous PR introduced a new compiler abstraction that tracks *all* known exports and re-exports of Angular traits. This PR harnesses that abstraction in the language service, in order to allow automatic imports of pipes.

dylhunn · 2023-01-12T17:59:29Z

caretaker: the failing windows CI job appears to be unrelated.

AndrewKushnir · 2023-01-12T21:46:49Z

This PR was merged into the repository by commit 4ae384f.

…8354) `MetadataReaderWithIndex.getKnown` currently returns an iterator. It will be easier to work with for upcoming usages if it returns an array instead. PR Close #48354

…suggest re-exports. (#48354) NgModules can re-export other NgModules, which transitively includes all traits exported by the re-exported NgModule. We want to be able to suggest *all* re-exports of a component when trying to auto-import it. Previously, we used an approximation when importing from NgModules: we looked at a Component's metadata, and just imported the declaring NgModule. However, this is not technically correct -- the declaring NgModule it is not necessarily the same one that exports it for the current scope. (Indeed, there could be multiple re-exports!) As a replacement, I have implemented a more general solution. This PR introduces a new class on the template type checker, called `NgModuleIndex`. When queried, it conducts a depth-first-search over the NgModule import/export graph, in order to find all NgModules anywhere in the current dependency graph, as well as all exports of those NgModules. This allows the language service to suggest all of the re-exports, in addition to the original export. PR Close #48354

…en its selector is used, both directly and via reexports. (#48354) A previous PR introduced a new compiler abstraction that tracks *all* known exports and re-exports of Angular traits. This PR harnesses that abstraction in the language service, in order to allow automatic imports of pipes. PR Close #48354

…ngular#48354) Currently, the auto-import tests are very difficult to debug, because the `expect`ations are buried several function calls deep, and also because the multi-line replacements are hard to deal with. The auto-import tests now use a different approach, which can be easily debugged from the failure message. In addition, several new tests have been added to cover more cases. PR Close angular#48354

…gular#48354) `MetadataReaderWithIndex.getKnown` currently returns an iterator. It will be easier to work with for upcoming usages if it returns an array instead. PR Close angular#48354

…suggest re-exports. (angular#48354) NgModules can re-export other NgModules, which transitively includes all traits exported by the re-exported NgModule. We want to be able to suggest *all* re-exports of a component when trying to auto-import it. Previously, we used an approximation when importing from NgModules: we looked at a Component's metadata, and just imported the declaring NgModule. However, this is not technically correct -- the declaring NgModule it is not necessarily the same one that exports it for the current scope. (Indeed, there could be multiple re-exports!) As a replacement, I have implemented a more general solution. This PR introduces a new class on the template type checker, called `NgModuleIndex`. When queried, it conducts a depth-first-search over the NgModule import/export graph, in order to find all NgModules anywhere in the current dependency graph, as well as all exports of those NgModules. This allows the language service to suggest all of the re-exports, in addition to the original export. PR Close angular#48354

…en its selector is used, both directly and via reexports. (angular#48354) A previous PR introduced a new compiler abstraction that tracks *all* known exports and re-exports of Angular traits. This PR harnesses that abstraction in the language service, in order to allow automatic imports of pipes. PR Close angular#48354

angular-automatic-lock-bot · 2023-02-12T16:06:17Z

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.}

dylhunn added the area: language-service Issues related to Angular's VS Code language service label Dec 5, 2022

ngbot bot added this to the Backlog milestone Dec 5, 2022

dylhunn mentioned this pull request Dec 5, 2022

Language Service: Automatic Import Improvements #47737

Closed

angular-robot bot added the detected: feature PR contains a feature commit label Dec 5, 2022

dylhunn force-pushed the ls-pipes branch 2 times, most recently from de4e81e to e5fc833 Compare December 5, 2022 02:27

dylhunn marked this pull request as ready for review December 5, 2022 02:27

dylhunn added action: review The PR is still awaiting reviews from at least one requested reviewer target: minor This PR is targeted for the next minor release labels Dec 5, 2022

pullapprove bot requested review from AndrewKushnir and alxhub December 5, 2022 02:28

dylhunn force-pushed the ls-pipes branch from e5fc833 to 7b6155e Compare December 5, 2022 02:28

dylhunn requested review from atscott and removed request for AndrewKushnir December 5, 2022 02:29

dylhunn force-pushed the ls-pipes branch 3 times, most recently from ad8fb05 to 49787c1 Compare December 5, 2022 03:07

dylhunn added the action: presubmit The PR is in need of a google3 presubmit label Dec 5, 2022

dylhunn force-pushed the ls-pipes branch from 49787c1 to 2281e7e Compare December 5, 2022 03:17

dylhunn added the action: rerun CI at HEAD label Dec 5, 2022

ngbot bot removed the action: rerun CI at HEAD label Dec 5, 2022

dylhunn added the action: rerun CI at HEAD label Dec 5, 2022

ngbot bot removed the action: rerun CI at HEAD label Dec 5, 2022

dylhunn force-pushed the ls-pipes branch from 2281e7e to cef8e4d Compare December 6, 2022 21:18

dylhunn force-pushed the ls-pipes branch from cef8e4d to cf00ccb Compare December 7, 2022 20:45

atscott approved these changes Dec 9, 2022

View reviewed changes

packages/language-service/src/codefixes/fix_missing_import.ts Outdated Show resolved Hide resolved

packages/language-service/src/codefixes/fix_missing_import.ts Outdated Show resolved Hide resolved

atscott approved these changes Dec 9, 2022

View reviewed changes

dylhunn requested a review from JoostK December 14, 2022 17:18

dylhunn force-pushed the ls-pipes branch from cf00ccb to 8195a2b Compare December 14, 2022 17:52

dylhunn force-pushed the ls-pipes branch from 511fbdc to 4d9a795 Compare January 9, 2023 23:41

dylhunn requested a review from alxhub January 10, 2023 00:19

dylhunn force-pushed the ls-pipes branch from 4d9a795 to 25fd43d Compare January 10, 2023 00:26

dylhunn added 2 commits January 10, 2023 13:51

refactor(compiler-cli): Make getKnown return an array of nodes.

f7bb7b6

`MetadataReaderWithIndex.getKnown` currently returns an iterator. It will be easier to work with for upcoming usages if it returns an array instead.

dylhunn added the action: rerun CI at HEAD label Jan 10, 2023

ngbot bot removed the action: rerun CI at HEAD label Jan 10, 2023

dylhunn added the action: rerun CI at HEAD label Jan 11, 2023

ngbot bot removed the action: rerun CI at HEAD label Jan 11, 2023

alxhub approved these changes Jan 12, 2023

View reviewed changes

dylhunn force-pushed the ls-pipes branch from 25fd43d to d3d2151 Compare January 12, 2023 17:27

dylhunn added 2 commits January 12, 2023 09:58

dylhunn force-pushed the ls-pipes branch from d3d2151 to 18b0681 Compare January 12, 2023 17:58

AndrewKushnir closed this in 2a4c5b4 Jan 12, 2023

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(language-service): Allow auto-imports of a pipe via quick fix when its selector is used.#48354

feat(language-service): Allow auto-imports of a pipe via quick fix when its selector is used.#48354
dylhunn wants to merge 4 commits intoangular:mainfrom
dylhunn:ls-pipes

dylhunn commented Dec 5, 2022 •

edited

Loading

Uh oh!

dylhunn commented Dec 7, 2022

Uh oh!

atscott left a comment

Uh oh!

Uh oh!

Uh oh!

atscott left a comment

Uh oh!

dylhunn commented Jan 9, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dylhunn commented Jan 12, 2023

Uh oh!

AndrewKushnir commented Jan 12, 2023

Uh oh!

angular-automatic-lock-bot bot commented Feb 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

dylhunn commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

refactor(language-service): Improve the quick fix auto-import tests.

refactor(compiler-cli): Make getKnown return an array of nodes.

feat(language-service): Introduce a new NgModuleIndex, and use it to suggest re-exports.

feat(language-service): Allow auto-imports of a pipe via quick fix when its selector is used, both directly and via reexports.

Uh oh!

dylhunn commented Dec 7, 2022

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

dylhunn commented Jan 9, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dylhunn commented Jan 12, 2023

Uh oh!

AndrewKushnir commented Jan 12, 2023

Uh oh!

angular-automatic-lock-bot bot commented Feb 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dylhunn commented Dec 5, 2022 •

edited

Loading

refactor(compiler-cli): Make `getKnown` return an array of nodes.