Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

atscott
Copy link
Contributor

@atscott atscott 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 August 13, 2019 19:29
@atscott atscott requested review from a team as code owners August 13, 2019 19:29
@ngbot ngbot bot modified the milestone: needsTriage Aug 13, 2019
@atscott atscott force-pushed the directives branch 2 times, most recently from 661fada to 1a0fec6 Compare August 13, 2019 19:38
@atscott atscott requested a review from a team as a code owner August 13, 2019 19:38
@atscott atscott force-pushed the directives branch 3 times, most recently from 6b87672 to 6871703 Compare August 13, 2019 20:04
alxhub
alxhub requested changes Aug 13, 2019
View reviewed changes
AndrewKushnir
AndrewKushnir requested changes Aug 13, 2019
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

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

@atscott atscott force-pushed the directives branch 2 times, most recently from 1a6a855 to 74c04cb Compare August 14, 2019 00:01
AndrewKushnir
AndrewKushnir approved these changes Aug 14, 2019
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@AndrewKushnir AndrewKushnir added the target: major This PR is targeted for the next major release label Aug 14, 2019
@crisbeto crisbeto mentioned this pull request Aug 14, 2019
Closed
@atscott atscott requested a review from a team as a code owner August 14, 2019 16:22
@atscott atscott added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Aug 14, 2019
@atscott atscott requested a review from alxhub August 14, 2019 16:32
@atscott atscott force-pushed the directives branch 4 times, most recently from 77a1183 to eb5b57e Compare August 14, 2019 20:41
@atscott atscott force-pushed the directives branch 5 times, most recently from fa976cb to c1d68c1 Compare August 14, 2019 23:46
alxhub
alxhub requested changes Aug 16, 2019
View reviewed changes
packages/core/testing/src/r3_test_bed_compiler.ts
@@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

packages/compiler-cli/src/ngtsc/metadata/src/dts.ts Outdated
@@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@atscott atscott force-pushed the directives branch 3 times, most recently from bcbc3e8 to fa23bb1 Compare August 16, 2019 21:55
@atscott
fix(ivy): Support selector-less directive as base classes
40fd1bb 
Following angular#31379, this adds support for directives without a selector to
Ivy.
@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 19, 2019
@IgorMinar IgorMinar removed the request for review from a team August 19, 2019 23:11
@atscott
Copy link
Contributor Author

atscott commented Aug 19, 2019

Ivy presubmit
VE presubmit

@atscott atscott 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 20, 2019
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
@atscott @sabeersulaiman
fix(ivy): Support selector-less directive as base classes (angular#32125
b9e8e9d 
)

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 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alxhub alxhub alxhub approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.
4 participants
@atscott @alxhub @AndrewKushnir @googlebot