Skip to content
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

fix(ivy): Support selector-less directive as base classes #32125

Closed
wants to merge 1 commit into from

Conversation

@atscott
Copy link
Contributor

commented Aug 13, 2019

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

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Only VE supports selector-less directives, per #31379.

Issue Number: N/A

What is the new behavior?

Ivy also supports selector-less directives

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@atscott atscott requested a review from alxhub Aug 13, 2019

@atscott atscott requested review from angular/fw-compiler as code owners Aug 13, 2019

@ngbot ngbot bot modified the milestone: needsTriage Aug 13, 2019

@googlebot googlebot added the cla: yes label Aug 13, 2019

@atscott atscott force-pushed the atscott:directives branch 2 times, most recently from 661fada to 1a0fec6 Aug 13, 2019

@atscott atscott requested a review from angular/fw-ngcc as a code owner Aug 13, 2019

@atscott atscott force-pushed the atscott:directives branch 3 times, most recently from 6b87672 to 6871703 Aug 13, 2019

@AndrewKushnir
Copy link
Contributor

left a comment

Looks great, just a couple nits. Thanks @atscott 👍

@atscott atscott force-pushed the atscott:directives branch 2 times, most recently from 1a6a855 to 74c04cb Aug 13, 2019

@AndrewKushnir
Copy link
Contributor

left a comment

LGTM 👍

@atscott atscott force-pushed the atscott:directives branch from 74c04cb to d077bcb Aug 14, 2019

@atscott atscott requested a review from angular/fw-public-api as a code owner Aug 14, 2019

@atscott atscott requested a review from alxhub Aug 14, 2019

@atscott atscott force-pushed the atscott:directives branch 4 times, most recently from 77a1183 to eb5b57e Aug 14, 2019

@atscott atscott force-pushed the atscott:directives branch 5 times, most recently from fa976cb to c1d68c1 Aug 14, 2019

@@ -299,7 +299,7 @@ export class R3TestBedCompiler {
this.pendingComponents.clear();

this.pendingDirectives.forEach(declaration => {
const metadata = this.resolvers.directive.resolve(declaration) !;
const metadata = this.resolvers.directive.resolve(declaration);

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 16, 2019

Contributor

+1 on the change, but this seems unrelated. Did our typings change in a way that made this no longer return null?

This comment has been minimized.

Copy link
@atscott

atscott Aug 16, 2019

Author Contributor

I think the difference is that this can actually return null now whereas before it never would. Since we allow @Directive(), this would return null but before we required an object and this call would always(?) return something. The metadata parameter of compileDirective call two lines below now allows null so I felt this was more accurate to what's possible instead of using the null assertion operator.

@@ -21,6 +21,11 @@ import {extractDirectiveGuards, extractReferencesFromType, readStringArrayType,
export class DtsMetadataReader implements MetadataReader {
constructor(private checker: ts.TypeChecker, private reflector: ReflectionHost) {}

isAbstractDirective(ref: Reference<ClassDeclaration>): boolean {
const meta = this.getDirectiveMetadata(ref);
return meta ? (meta.selector === null || meta.selector === '') : false;

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 16, 2019

Contributor

Explicitly check for nullness of meta, don't rely on truthiness.

I think we should treat selector: '' as an error and not allow it to be produced in the first place. Thoughts?

This comment has been minimized.

Copy link
@atscott

atscott Aug 16, 2019

Author Contributor

Agreed. We actually do throw errors elsewhere if you explicitly set selector: ''

@atscott atscott force-pushed the atscott:directives branch 3 times, most recently from bcbc3e8 to fa23bb1 Aug 16, 2019

@alxhub
alxhub approved these changes Aug 19, 2019
fix(ivy): Support selector-less directive as base classes
Following #31379, this adds support for directives without a selector to
Ivy.

@atscott atscott force-pushed the atscott:directives branch from fa23bb1 to 40fd1bb Aug 19, 2019

@IgorMinar IgorMinar removed the request for review from angular/fw-public-api Aug 19, 2019

@atscott

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

ngdevelop-tech added a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
fix(ivy): Support selector-less directive as base classes (angular#32125
)

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

PR Close angular#32125
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
fix(ivy): Support selector-less directive as base classes (angular#32125
)

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

PR Close angular#32125
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.