fix(ivy): allow abstract directives to have an invalid constructor by JoostK · Pull Request #32987 · angular/angular

JoostK · 2019-10-03T20:30:34Z

For abstract directives, i.e. directives without a selector, it may
happen that their constructor is called explicitly from a subclass,
hence its parameters are not required to be valid for Angular's DI
purposes. Prior to this commit however, having an abstract directive
with a constructor that has parameters that are not eligible for
Angular's DI would produce a compilation error.

A similar scenario may occur for @Injectables, where an explicit
use* definition allows for the constructor to be irrelevant. For
example, the situation where useFactory is specified allows for the
constructor to be called explicitly with any value, so its constructor
parameters are not required to be valid. For @Injectables this is
handled by generating a DI factory function that throws.

This commit implements the same solution for abstract directives, such
that a compilation error is avoided while still producing an error at
runtime if the type is instantiated implicitly by Angular's DI
mechanism.

Fixes #32981

TODO

Adjust error message for abstract directives, it currently refers to @Injectable

JoostK · 2019-10-03T20:37:34Z

packages/compiler-cli/src/ngtsc/annotations/src/util.ts

Note to self: maybe this should not have been extracted into a function, but rather repeated at the call sites as it previously was. The reason being that the pending TODO is not applicable to abstract directives, so this shared function may not be desirable.

Nope, this LGTM :) I just changed the comments.

JoostK · 2019-10-04T08:17:00Z

One more thing we should take into account: throwing an error with a descriptive message is quite heavy on code size, so we may want to look into alternatives.

trotyl · 2019-10-04T13:25:00Z

For abstract directives, i.e. directives without a selector

This left no rooms for possible runtime directive in future, which could always be selector-less. Selector-less and base/abstract are conceptually not the same thing, a concrete component can be selector-less, as long as not meant for template usage (entry component only).

JoostK · 2019-10-10T20:06:54Z

We came up with an idea to generate ngFactoryDef = null; instead of a function that always throws, as that is better suitable regarding code size and still allows an error with descriptive message to be produced form within the runtime itself. This avoids duplicating the error message potentially multiple times in a single bundle, and the message in the runtime can be shortened/left out in prod builds.

alxhub · 2019-10-25T01:10:08Z

Presubmit
Ivy Presubmit

JoostK · 2019-10-25T08:12:13Z

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

I'm not sure if I wrote this or this was added by @alxhub, but we should definitely have a test with strictInjectionParameters: true to know that it will not fail at compile time for abstract directives, instead generating ɵɵinvalidFactory (which is unlike the result for concrete directives, which should fail to compile in that case).

JoostK · 2019-10-25T08:17:05Z

packages/compiler/src/render3/r3_factory.ts

I changed this to become an enum to be able to tweak the error message, now that we generate the invalidFactory instruction that is not strictly necessary anymore. However, I still like this change so it can stay in, but be aware that these changes somehow causes breaking tests that end up with an infinite recursive call during change detection. This is still the case today, and I have no clue how this happens.

Fixed. JIT directive compilation was adding a factory intended for pipes, which uses a different ChangeDetectorRef implementation.

AndrewKushnir · 2019-10-25T16:39:46Z

@JoostK @alxhub there are some failing CircleCI jobs, could you please have a look? Thank you.

For abstract directives, i.e. directives without a selector, it may happen that their constructor is called explicitly from a subclass, hence its parameters are not required to be valid for Angular's DI purposes. Prior to this commit however, having an abstract directive with a constructor that has parameters that are not eligible for Angular's DI would produce a compilation error. A similar scenario may occur for `@Injectable`s, where an explicit `use*` definition allows for the constructor to be irrelevant. For example, the situation where `useFactory` is specified allows for the constructor to be called explicitly with any value, so its constructor parameters are not required to be valid. For `@Injectable`s this is handled by generating a DI factory function that throws. This commit implements the same solution for abstract directives, such that a compilation error is avoided while still producing an error at runtime if the type is instantiated implicitly by Angular's DI mechanism. Fixes angular#32981

alxhub · 2019-10-25T18:17:58Z

Presubmit
Ivy Presubmit

alxhub · 2019-10-25T18:59:01Z

Caretaker: public API is unaffected (addition of a private API)

angular-automatic-lock-bot · 2019-11-25T16:11:57Z

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

ngbot bot modified the milestone: Backlog Oct 3, 2019

googlebot added the cla: yes label Oct 3, 2019

ngbot bot modified the milestone: Backlog Oct 3, 2019

JoostK commented Oct 3, 2019

View reviewed changes

JoostK force-pushed the ngtsc-abstract-dir-ctor branch 2 times, most recently from 3ddfa57 to b9c1c31 Compare October 7, 2019 19:04

crisbeto mentioned this pull request Oct 24, 2019

refactor(ivy): remove ngBaseDef #33264

Closed

alxhub force-pushed the ngtsc-abstract-dir-ctor branch from b9c1c31 to 75bc9d7 Compare October 25, 2019 00:27

alxhub marked this pull request as ready for review October 25, 2019 00:32

alxhub requested review from a team October 25, 2019 00:32

alxhub force-pushed the ngtsc-abstract-dir-ctor branch from 75bc9d7 to 341fe74 Compare October 25, 2019 01:07

alxhub approved these changes Oct 25, 2019

View reviewed changes

JoostK commented Oct 25, 2019

View reviewed changes

alxhub force-pushed the ngtsc-abstract-dir-ctor branch from 341fe74 to ee02310 Compare October 25, 2019 16:43

JoostK requested a review from a team October 25, 2019 16:43

alxhub force-pushed the ngtsc-abstract-dir-ctor branch from ee02310 to fc8481b Compare October 25, 2019 16:51

AndrewKushnir removed the state: WIP label Oct 25, 2019

alxhub added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Oct 25, 2019

AndrewKushnir removed the cla: yes label Oct 25, 2019

googlebot added the cla: yes label Oct 25, 2019

AndrewKushnir closed this in 8d15bfa Oct 25, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 25, 2019

Conversation

JoostK commented Oct 3, 2019 • edited by alxhub Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoostK Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

alxhub Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

JoostK commented Oct 4, 2019

Uh oh!

trotyl commented Oct 4, 2019

Uh oh!

JoostK commented Oct 10, 2019

Uh oh!

alxhub commented Oct 25, 2019

Uh oh!

JoostK Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

alxhub Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

JoostK Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

alxhub Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir commented Oct 25, 2019

Uh oh!

alxhub commented Oct 25, 2019

Uh oh!

alxhub commented Oct 25, 2019

Uh oh!

angular-automatic-lock-bot bot commented Nov 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JoostK commented Oct 3, 2019 •

edited by alxhub

Loading