refactor(compiler): add support for host directives by crisbeto · Pull Request #46868 · angular/angular

crisbeto · 2022-07-18T11:28:08Z

This is the compile-time implementation of the hostDirectives feature plus a little bit of runtime code to illustrate how the newly-generated code will plug into the runtime. It works by creating a call to the new ɵɵHostDirectivesFeature feature whenever a directive has a hostDirectives field. Afterwards ɵɵHostDirectivesFeature will patch a new function onto the directive definition that will be invoked during directive matching.

For example, if we take the following definition:

@Directive({
  hostDirectives: [HostA, {directive: HostB, inputs: ['input: alias']}]
})
class MyDir {}

Will compile to:

MyDir.ɵdir = ɵɵdefineComponent({
  features: [ɵɵHostDirectivesFeature([HostA, {
    directive: HostB,
    inputs: ['input', 'alias']
  }])]
});

The template type checking is implemented during directive matching by adding the host directives applied on the host to the array of matched directives whenever the host is matched in a template.

Relates to #8785.

alxhub

First round of review! Overall this is in pretty good shape.

A few meta-comments about the PR as a whole:

Ideally, we would split this up into multiple commits.

For example, one commit could deal with adding the extra metadata to .d.ts files (starting as never always), another could do the parsing, another the template type-checking, etc.

This should not be a feat commit if the feature is not actually implemented.

Instead, make it a refactor commit. We don't want any entry in the changelog until the feature is usable to users. For a big implementation like this, we can have multiple PRs adding implementation details that aren't exposed, and then a final feat commit which only makes the API public.

On that note, we shouldn't also add hostDirectives to the public @Directive metadata just yet.

That is, we shouldn't change the public API and allow users to access the feature until it's fully implemented and working.

crisbeto · 2022-07-21T11:08:01Z

Regarding splitting it up into multiple commits: I agree that this is a huge change, but I ended up going with the single commit, because the majority of it are the actual tests, not the compiler code, and in order to make some tests work, most of the compiler changes would have to be in place.

Regarding the commit message: I can target this to the main branch so it won't show up in the changelog until we release it. Alternatively I can target it to a feature branch instead.

Regarding exposing the new API in the @Directive metadata: I exposed it, because some of the tests are basically testing the entire pipeline end-to-end and I wanted to avoid compilation errors. The code won't be released for a while and even if it was passed in, the runtime isn't in place so nothing will happen.

crisbeto · 2022-07-23T16:03:17Z

@alxhub I've addressed the feedback above and the things we talked about in person. All the changes are in the second fixup commit. Here's a high level overview of what has changed:

The commit message was switched to refactor so it doesn't end up in the changelog until the feature has been implemented completely.
Instead of preserving forwardRef calls, the compiler will generate a function when there are forward references.
hostDirectives has been removed from the public API of @Directive and @Component until we're done implementing the feature.
I've added some logic to filter out the host directives from the component template dependencies, as we discussed.
The compiler now produces diagnostics for problems in the host directives definition, instead of throwing errors. Also the validation happens in the validate method of HostDirectivesResolver instead of in resolve.
I had to expand the type on a couple of utilities that deal with diagnostics from ts.Expression to ts.Node so that I could produce diagnostics with the correct context for host directives. As far as I can tell, we weren't using anything specific to Expression anyway.
The comments around return types and moving literals into dedicated types have been resolved.

`ComponentRef.setInput` works by calling `setInputsForProperty` internally. It can be problematic, because it will set the all inputs on the specific node matching the passed-in name, including other directives that aren't associated with the `ComponentRef`. Currently this isn't a problem, because the only way to get a `ComponentRef` is as a result of `ComponentFactory.create` and there is no way to apply other directives through there, however I see a couple of ways that it can manifest itself in the future: 1. Once the runtime for host directives is implemented (after angular#46868), users will be able to apply directives to the root component. 2. If we were to make it possible to inject `ComponentRef` in the future. These changes add some extra logic so that the input is only assigned to the component instance that is associated with the `ComponentRef`.

alxhub

Reviewed-for: public-api

pkozlowski-opensource

LGTM

Reviewed-for: public-api

@directive

This is the compile-time implementation of the `hostDirectives` feature plus a little bit of runtime code to illustrate how the newly-generated code will plug into the runtime. It works by creating a call to the new `ɵɵHostDirectivesFeature` feature whenever a directive has a `hostDirectives` field. Afterwards `ɵɵHostDirectivesFeature` will patch a new function onto the directive definition that will be invoked during directive matching. For example, if we take the following definition: ```ts @directive({ hostDirectives: [HostA, {directive: HostB, inputs: ['input: alias']}] }) class MyDir {} ``` Will compile to: ```js MyDir.ɵdir = ɵɵdefineComponent({ features: [ɵɵHostDirectivesFeature([HostA, { directive: HostB, inputs: { input: "alias" } }])] }); ``` The template type checking is implemented during directive matching by adding the host directives applied on the host to the array of matched directives whenever the host is matched in a template. Relates to angular#8785.

jessicajaniuk

reviewed-for: public-api

alxhub · 2022-08-22T23:00:38Z

This PR was merged into the repository by commit 54ceed5.

angular-automatic-lock-bot · 2022-09-22T16:07:01Z

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

crisbeto force-pushed the host-directives-compiler branch 4 times, most recently from 50f29c1 to 554e814 Compare July 18, 2022 12:20

crisbeto 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 Jul 18, 2022

crisbeto marked this pull request as ready for review July 18, 2022 12:41

pullapprove Bot requested review from atscott and gkalpak July 18, 2022 12:41

crisbeto requested a review from alxhub July 18, 2022 12:42

This comment was marked as off-topic.

Sign in to view

jessicajaniuk added the area: compiler Issues related to `ngc`, Angular's template compiler label Jul 18, 2022

ngbot Bot added this to the Backlog milestone Jul 18, 2022

This comment was marked as off-topic.

Sign in to view

alxhub requested changes Jul 21, 2022

View reviewed changes

crisbeto changed the title ~~feat(compiler): add support for host directives~~ refactor(compiler): add support for host directives Jul 22, 2022

crisbeto force-pushed the host-directives-compiler branch 3 times, most recently from 1b42abd to 9a0bbf5 Compare July 23, 2022 15:47

crisbeto mentioned this pull request Jul 31, 2022

fix(core): only set input on specific instance in ComponentRef.setInput #46997

Closed

crisbeto force-pushed the host-directives-compiler branch 2 times, most recently from 4ae9164 to 3bfea17 Compare August 3, 2022 09:29

pullapprove Bot requested review from jessicajaniuk and pkozlowski-opensource August 22, 2022 17:21

alxhub approved these changes Aug 22, 2022

View reviewed changes

pkozlowski-opensource approved these changes Aug 22, 2022

View reviewed changes

alxhub removed request for AndrewKushnir and gkalpak August 22, 2022 17:31

crisbeto added 8 commits August 22, 2022 19:45

fixup! refactor(compiler): add support for host directives

e6e7b3b

fixup! refactor(compiler): add support for host directives

efabbfa

fixup! refactor(compiler): add support for host directives

77f1987

fixup! refactor(compiler): add support for host directives

2be09d0

fixup! refactor(compiler): add support for host directives

4b45ec1

fixup! refactor(compiler): add support for host directives

9c37aae

fixup! refactor(compiler): add support for host directives

454dbd1

crisbeto force-pushed the host-directives-compiler branch from a0dff5f to 454dbd1 Compare August 22, 2022 17:47

crisbeto self-assigned this Aug 22, 2022

jessicajaniuk approved these changes Aug 22, 2022

View reviewed changes

crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Aug 22, 2022

crisbeto removed their assignment Aug 22, 2022

alxhub added target: minor This PR is targeted for the next minor release and removed target: major This PR is targeted for the next major release labels Aug 22, 2022

alxhub closed this in 54ceed5 Aug 22, 2022

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 22, 2022

Conversation

crisbeto commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

alxhub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crisbeto commented Jul 21, 2022

Uh oh!

crisbeto commented Jul 23, 2022

Uh oh!

alxhub left a comment

Choose a reason for hiding this comment

Uh oh!

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

Uh oh!

jessicajaniuk left a comment

Choose a reason for hiding this comment

Uh oh!

alxhub commented Aug 22, 2022

Uh oh!

angular-automatic-lock-bot Bot commented Sep 22, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

crisbeto commented Jul 18, 2022 •

edited

Loading