feat(ivy): Add AOT handling for bare classes with Input and Output de… by benlesh · Pull Request #25367 · angular/angular

benlesh · 2018-08-07T22:59:51Z

…corators

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] 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?

Currently, ivy does not handle inheritance properly via AOt in situations where a base class has @Input or @Output properties, but no @Component or @Directive decorator on the class.

Issue Number: N/A

What is the new behavior?

The AOT compiler will add ngBaseDef static property, but only when appropriate.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Related to #25129

mary-poppins · 2018-08-07T23:14:48Z

You can preview 093ce73 at https://pr25367-093ce73.ngbuilds.io/.

JoostK · 2018-08-07T23:24:17Z

Should we really break here, wouldn't that mean any remaining inputs/outputs won't be collected?

Oops, thanks, that was left over from a previous pass. Adding an additional test as well.

JoostK · 2018-08-07T23:25:18Z

We should test their imported module is angular core.

JoostK · 2018-08-07T23:26:31Z

How about HostBinding/HostListener? Shouldn't they be included in the base definition just as well?

That work is separate.

mary-poppins · 2018-08-08T00:42:09Z

You can preview 1d6e431 at https://pr25367-1d6e431.ngbuilds.io/.

mary-poppins · 2018-08-08T01:43:52Z

You can preview 7c7a5dd at https://pr25367-7c7a5dd.ngbuilds.io/.

mary-poppins · 2018-08-08T01:52:40Z

You can preview ea4c153 at https://pr25367-ea4c153.ngbuilds.io/.

mary-poppins · 2018-08-08T15:16:22Z

You can preview 2f18bb5 at https://pr25367-2f18bb5.ngbuilds.io/.

mary-poppins · 2018-08-08T15:32:22Z

You can preview 462358c at https://pr25367-462358c.ngbuilds.io/.

benlesh · 2018-08-08T17:02:34Z

g3sync http://test/OCL:207796611:BASE:207902428:1533747689736:442c7ae7

trotyl · 2018-08-09T02:58:17Z

angular/packages/core/src/metadata/directives.ts

Line 790 in 6c4da9d

if (!constructor.hasOwnProperty('ngBaseDef')) {

Possibly fix this as well? ngBaseDef is minifiable.

alxhub · 2018-08-09T09:07:59Z

Import should not be deep. It can be added to the ../../ngtsc/annotations import above.

alxhub · 2018-08-09T09:09:11Z

Can you convince the auto-formatter to format this in a more readable way?

alxhub · 2018-08-09T09:14:47Z

I believe this field should become match: M.

alxhub · 2018-08-09T09:17:29Z

"let that DecoratorHandler handle this."

alxhub · 2018-08-09T09:17:58Z

Don't cast it to a string, check if it is a string and throw if it isn't.

alxhub · 2018-08-09T09:19:14Z

alxhub · 2018-08-09T09:22:58Z

Rename decorator to metadata - it should also be of type M.

alxhub · 2018-08-09T09:23:54Z

alxhub · 2018-08-09T09:24:24Z

expect(result.source).not.toContain('ngBaseDef')

alxhub · 2018-08-09T09:24:31Z

alxhub · 2018-08-09T09:25:25Z

Nice work, this looks generally good. Just a few comments to address and then I'll approve it.

mary-poppins · 2018-08-09T16:29:27Z

You can preview c9015dc at https://pr25367-c9015dc.ngbuilds.io/.

mary-poppins · 2018-08-09T22:51:56Z

You can preview 70b1b83 at https://pr25367-70b1b83.ngbuilds.io/.

alan-agius4 · 2018-08-12T10:12:16Z

+/**
+ * Used to get the minified alias of ngBaseDef
+ */
+const NG_BASE_DEF = Object.keys({ngBaseDef: true})[0];


Why not const NG_BASE_DEF = 'ngBaseDef' directly?

@alan-agius4 For Closure Compiler, which may minify the ngBaseDef property to something else. Literal strings however won't be affected by this minification step, so it is actually used as a property here for it to undergo the same minification procedure.

In other parts of the code base a function getClosureSafeProperty is used for the same purpose. That makes this more explicit, but I actually like what @benlesh has done here as it's much shorter.

@JoostK, I see, interesting :) Thanks for the explanation.

…corators

…tput decorators

mary-poppins · 2018-08-14T20:47:44Z

You can preview 0abdcbd at https://pr25367-0abdcbd.ngbuilds.io/.

…tput decorators

mary-poppins · 2018-08-14T21:32:22Z

You can preview f1cbae0 at https://pr25367-f1cbae0.ngbuilds.io/.

ngbot · 2018-08-14T23:34:05Z

I see that you just added the PR action: merge label, but the following checks are still failing:
forbidden label detected: PR action: review
status "code-review/pullapprove" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

angular-automatic-lock-bot · 2019-09-13T23:22:11Z

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

googlebot added the cla: yes label Aug 7, 2018

benlesh force-pushed the ivy-aot-basedef branch from f793dfe to 093ce73 Compare August 7, 2018 23:00

benlesh added target: major This PR is targeted for the next major release action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 7, 2018

benlesh removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Aug 7, 2018

JoostK reviewed Aug 7, 2018

View reviewed changes

benlesh force-pushed the ivy-aot-basedef branch from 093ce73 to 1d6e431 Compare August 8, 2018 00:33

benlesh force-pushed the ivy-aot-basedef branch from 7c7a5dd to ea4c153 Compare August 8, 2018 01:44

benlesh force-pushed the ivy-aot-basedef branch from ea4c153 to 2f18bb5 Compare August 8, 2018 15:07

benlesh force-pushed the ivy-aot-basedef branch from 2f18bb5 to 462358c Compare August 8, 2018 15:21

benlesh added the action: review The PR is still awaiting reviews from at least one requested reviewer label Aug 8, 2018

benlesh requested a review from alxhub August 8, 2018 22:29

alxhub requested changes Aug 9, 2018

View reviewed changes

alxhub approved these changes Aug 9, 2018

View reviewed changes

benlesh force-pushed the ivy-aot-basedef branch from c9015dc to 70b1b83 Compare August 9, 2018 22:43

alan-agius4 reviewed Aug 12, 2018

View reviewed changes

benlesh added 2 commits August 14, 2018 13:35

feat(ivy): Add AOT handling for bare classes with Input and Output de…

9df7660

…corators

fixup! feat(ivy): Add AOT handling for bare classes with Input and Ou…

0abdcbd

…tput decorators

benlesh force-pushed the ivy-aot-basedef branch from 70b1b83 to 0abdcbd Compare August 14, 2018 20:35

fixup! feat(ivy): Add AOT handling for bare classes with Input and Ou…

f1cbae0

…tput decorators

benlesh added the action: merge The PR is ready for merge by the caretaker label Aug 14, 2018

benlesh closed this in a0a29fd Aug 14, 2018

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 13, 2019

Conversation

benlesh commented Aug 7, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

mary-poppins commented Aug 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Aug 8, 2018

Uh oh!

mary-poppins commented Aug 8, 2018

Uh oh!

mary-poppins commented Aug 8, 2018

Uh oh!

mary-poppins commented Aug 8, 2018

Uh oh!

mary-poppins commented Aug 8, 2018

Uh oh!

benlesh commented Aug 8, 2018

Uh oh!

trotyl commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alxhub commented Aug 9, 2018

Uh oh!

trotyl commented Aug 9, 2018 •

edited

Loading

JoostK Aug 12, 2018 •

edited

Loading

alan-agius4 Aug 12, 2018 •

edited

Loading