Skip to content

feat(compiler): allow selector-less directives as base classes #31379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Jul 1, 2019

In Angular today, the following pattern works:

export class BaseDir {
  constructor(@Inject(ViewContainerRef) protected vcr: ViewContainerRef) {}
}

@Directive({
  selector: '[child]',
})
export class ChildDir extends BaseDir {
  // constructor inherited from BaseDir
}

A decorated child class can inherit a constructor from an undecorated base
class, so long as the base class has metadata of its own (for JIT mode).
This pattern works regardless of metadata in AOT.

In Angular Ivy, this pattern does not work: without the @directive
annotation identifying the base class as a directive, information about its
constructor parameters will not be captured by the Ivy compiler. This is a
result of Ivy's locality principle, which is the basis behind a number of
compilation optimizations.

As a solution, @directive() without a selector will be interpreted as a
"directive base class" annotation. Such a directive cannot be declared in an
NgModule, but can be inherited from.

@alxhub alxhub force-pushed the ngc/ivy/abstract-dir branch 3 times, most recently from 54c01b1 to f3e64f1 Compare July 2, 2019 19:58
@alxhub alxhub marked this pull request as ready for review July 2, 2019 21:20
@alxhub alxhub requested review from a team as code owners July 2, 2019 21:20
@alxhub alxhub added the target: major This PR is targeted for the next major release label Jul 2, 2019
@alxhub
Copy link
Member Author

alxhub commented Jul 2, 2019

Presubmit

@jasonaden jasonaden added the area: compiler Issues related to `ngc`, Angular's template compiler label Jul 9, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jul 9, 2019
@mhevery mhevery added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 12, 2019
@alxhub alxhub force-pushed the ngc/ivy/abstract-dir branch from f3e64f1 to 8ff47db Compare July 15, 2019 17:37
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM, one nit

directives.push(symbol);
} else {
abstractDirectives.push(symbol);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can we flip the ternary so it's not "if-not/else" ?

           if (metadataResolver.isAbstractDirective(symbol)) {
              abstractDirectives.push(symbol);
            } else {
              directives.push(symbol);
            }

Copy link
Member Author

Choose a reason for hiding this comment

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

The general rule I try to use in the compiler is that for simple if-else blocks, the common case is the first branch. So in this case, most directives are plain directives and flow through the if section, and the else section handles the occasional abstract directive. That does make the conditional a little trickier to parse, though.

I added some extra comments to clarify the two bodies here :)

@alxhub alxhub force-pushed the ngc/ivy/abstract-dir branch from 8ff47db to 8fcb1fa Compare July 29, 2019 17:57
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

lgtm for api changes

@alxhub alxhub added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Aug 2, 2019
@alxhub alxhub added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Aug 9, 2019
@alxhub
Copy link
Member Author

alxhub commented Aug 9, 2019

Caretaker: reviews in place

@kara kara closed this in f90c7a9 Aug 9, 2019
kara added a commit to kara/angular that referenced this pull request Aug 10, 2019
@kara kara reopened this Aug 10, 2019
@AndrewKushnir AndrewKushnir removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Aug 14, 2019
@AndrewKushnir AndrewKushnir removed action: presubmit The PR is in need of a google3 presubmit state: blocked labels Aug 14, 2019
@AndrewKushnir AndrewKushnir added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Aug 14, 2019
atscott added a commit to atscott/angular that referenced this pull request Aug 19, 2019
Following angular#31379, this adds support for directives without a selector to
Ivy.
AndrewKushnir pushed a commit that referenced this pull request Aug 20, 2019
Following #31379, this adds support for directives without a selector to
Ivy.

PR Close #32125
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
…ar#31379)

In Angular today, the following pattern works:

```typescript
export class BaseDir {
  constructor(@Inject(ViewContainerRef) protected vcr: ViewContainerRef) {}
}

@directive({
  selector: '[child]',
})
export class ChildDir extends BaseDir {
  // constructor inherited from BaseDir
}
```

A decorated child class can inherit a constructor from an undecorated base
class, so long as the base class has metadata of its own (for JIT mode).
This pattern works regardless of metadata in AOT.

In Angular Ivy, this pattern does not work: without the @directive
annotation identifying the base class as a directive, information about its
constructor parameters will not be captured by the Ivy compiler. This is a
result of Ivy's locality principle, which is the basis behind a number of
compilation optimizations.

As a solution, @directive() without a selector will be interpreted as a
"directive base class" annotation. Such a directive cannot be declared in an
NgModule, but can be inherited from. To implement this, a few changes are
made to the ngc compiler:

* the error for a selector-less directive is now generated when an NgModule
  declaring it is processed, not when the directive itself is processed.
* selector-less directives are not tracked along with other directives in
  the compiler, preventing other errors (like their absence in an NgModule)
  from being generated from them.

PR Close angular#31379
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
…ar#31379)

In Angular today, the following pattern works:

```typescript
export class BaseDir {
  constructor(@Inject(ViewContainerRef) protected vcr: ViewContainerRef) {}
}

@directive({
  selector: '[child]',
})
export class ChildDir extends BaseDir {
  // constructor inherited from BaseDir
}
```

A decorated child class can inherit a constructor from an undecorated base
class, so long as the base class has metadata of its own (for JIT mode).
This pattern works regardless of metadata in AOT.

In Angular Ivy, this pattern does not work: without the @directive
annotation identifying the base class as a directive, information about its
constructor parameters will not be captured by the Ivy compiler. This is a
result of Ivy's locality principle, which is the basis behind a number of
compilation optimizations.

As a solution, @directive() without a selector will be interpreted as a
"directive base class" annotation. Such a directive cannot be declared in an
NgModule, but can be inherited from. To implement this, a few changes are
made to the ngc compiler:

* the error for a selector-less directive is now generated when an NgModule
  declaring it is processed, not when the directive itself is processed.
* selector-less directives are not tracked along with other directives in
  the compiler, preventing other errors (like their absence in an NgModule)
  from being generated from them.

PR Close angular#31379
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
)

Following angular#31379, this adds support for directives without a selector to
Ivy.

PR Close angular#32125
@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 Sep 15, 2019
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 cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: medium target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants