Skip to content

refactor(compiler): rework and expose APIs to be used in schematics#48730

Closed
crisbeto wants to merge 3 commits intoangular:mainfrom
crisbeto:schematic-type-checker-apis
Closed

refactor(compiler): rework and expose APIs to be used in schematics#48730
crisbeto wants to merge 3 commits intoangular:mainfrom
crisbeto:schematic-type-checker-apis

Conversation

@crisbeto
Copy link
Member

Reworks some of the existing compiler APIs to make them easier to use in a schematic and exposes a few new ones to surface information we already had. High-level list of changes:

  • getPotentialImportsFor now requires a class reference, instead of a PotentialDirective | PotentialPipe.
  • New getNgModuleMetadata method has been added to the type checker.
  • New getPipeMetadata method has been added to the type checker.
  • New getUsedDirectives method has been added to the type checker.
  • New getUsedPipes method has been added to the type checker.
  • The decorator property was exposed on the TypeCheckableDirectiveMeta. The property was already present at runtime, but it wasn't specified on the interface.

@crisbeto crisbeto force-pushed the schematic-type-checker-apis branch 2 times, most recently from ca7d7f6 to effa7a7 Compare January 13, 2023 15:05
@crisbeto crisbeto requested a review from dylhunn January 13, 2023 15:37
@crisbeto crisbeto marked this pull request as ready for review January 13, 2023 15:37
@pullapprove pullapprove bot requested a review from atscott January 13, 2023 15:37
@dylhunn dylhunn added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jan 13, 2023
@dylhunn
Copy link
Contributor

dylhunn commented Jan 13, 2023

Thanks, reviewing this today.

Copy link
Contributor

@dylhunn dylhunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with notes above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a new reference to perform the lookup works fine for locally declared ngModules, but may not work for ngModules coming from external dts file (i.e. anything not in the current compilation unit). This also applies to the other get*Metadata methods. I'm not sure if this matters to you -- for more details, look in the dts metadata reader.

Copy link
Member Author

@crisbeto crisbeto Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really matter for my case. FWIW, I was following what we were doing in getDirectiveMetadata which has been there for a while.

Reworks some of the existing compiler APIs to make them easier to use in a schematic and exposes a few new ones to surface information we already had. High-level list of changes:
* `getPotentialImportsFor` now requires a class reference, instead of a `PotentialDirective | PotentialPipe`.
* New `getNgModuleMetadata` method has been added to the type checker.
* New `getPipeMetadata` method has been added to the type checker.
* New `getUsedDirectives` method has been added to the type checker.
* New `getUsedPipes` method has been added to the type checker.
* The `decorator` property was exposed on the `TypeCheckableDirectiveMeta`. The property was already present at runtime, but it wasn't specified on the interface.
@crisbeto crisbeto added area: compiler Issues related to `ngc`, Angular's template compiler target: minor This PR is targeted for the next minor release labels Jan 13, 2023
@ngbot ngbot bot modified the milestone: Backlog Jan 13, 2023
@crisbeto crisbeto force-pushed the schematic-type-checker-apis branch from effa7a7 to 470b2c5 Compare January 13, 2023 19:13
@crisbeto
Copy link
Member Author

Caretaker note: the presubmit passed after a re-run (see http://test/OCL:501899200:BASE:501909214:1673640872156:74595b02). I'm not sure why the status is set to unknown.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 13, 2023
@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit 6beff5e.

trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
…ngular#48730)

Reworks some of the existing compiler APIs to make them easier to use in a schematic and exposes a few new ones to surface information we already had. High-level list of changes:
* `getPotentialImportsFor` now requires a class reference, instead of a `PotentialDirective | PotentialPipe`.
* New `getNgModuleMetadata` method has been added to the type checker.
* New `getPipeMetadata` method has been added to the type checker.
* New `getUsedDirectives` method has been added to the type checker.
* New `getUsedPipes` method has been added to the type checker.
* The `decorator` property was exposed on the `TypeCheckableDirectiveMeta`. The property was already present at runtime, but it wasn't specified on the interface.

PR Close angular#48730
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants