feat(compiler-cli): exclude abstract classes from `strictInjectionPar…#44615
feat(compiler-cli): exclude abstract classes from `strictInjectionPar…#44615JoostK wants to merge 1 commit intoangular:mainfrom
Conversation
72cc61c to
7f30792
Compare
packages/compiler-cli/src/ngtsc/annotations/common/src/diagnostics.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
We used to report this report this error for abstract classes and I haven't changed this, but I'm wondering if we should exempt abstract directives from this error.
There was a problem hiding this comment.
Interestingly, we don't check useClass here like we check providers in the providers array! 😱
efbc3f0 to
b1db475
Compare
jessicajaniuk
left a comment
There was a problem hiding this comment.
reviewed-for: public-api
b1db475 to
7be53d8
Compare
|
@JoostK It looks like this hasn't been updated in a while. Is this a PR we'd like to merge for 14? |
It's ready to go from my side, just awaiting review. I'm afraid we're short on time to get this LGTM'd in time. |
packages/compiler-cli/src/ngtsc/annotations/common/src/diagnostics.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Should this call registerInjectable before returning?
There was a problem hiding this comment.
Hmm yeah that might make sense to avoid potential repeated work 👍
AndrewKushnir
left a comment
There was a problem hiding this comment.
Reviewed-for: public-api
dylhunn
left a comment
There was a problem hiding this comment.
reviewed-for: public-api
48d4344 to
4714782
Compare
|
TGP is running. |
|
We've resolved all the TGP issues inside of google3, and have inflight CLs now. |
…meters` requirement In AOT compilations, the `strictInjectionParameters` compiler option can be enabled to report errors when an `@Injectable` annotated class has a constructor with parameters that do not provide an injection token, e.g. only a primitive type or interface. Since Ivy it's become required that any class with Angular behavior (e.g. the `ngOnDestroy` lifecycle hook) is decorated using an Angular decorator, which meant that `@Injectable()` may need to have been added to abstract base classes. Doing so would then report an error if `strictInjectionParameters` is enabled, if the abstract class has an incompatible constructor for DI purposes. This may be fine though, as a subclass may call the constructor explicitly without relying on Angular's DI mechanism. Therefore, this commit excludes abstract classes from the `strictInjectionParameters` check. This avoids an error from being reported at compile time. If the constructor ends up being used by Angular's DI system at runtime, then the factory function of the abstract class will throw an error by means of the `ɵɵinvalidFactory` instruction. In addition to the runtime error, this commit also analyzes the inheritance chain of an injectable without a constructor to verify that their inherited constructor is valid. BREAKING CHANGE: Invalid constructors for DI may now report compilation errors When a class inherits its constructor from a base class, the compiler may now report an error when that constructor cannot be used for DI purposes. This may either be because the base class is missing an Angular decorator such as `@Injectable()` or `@Directive()`, or because the constructor contains parameters which do not have an associated token (such as primitive types like `string`). These situations used to behave unexpectedly at runtime, where the class may be constructed without any of its constructor parameters, so this is now reported as an error during compilation. Any new errors that may be reported because of this change can be resolved either by decorating the base class from which the constructor is inherited, or by adding an explicit constructor to the class for which the error is reported. Closes angular#37914
4714782 to
699f9b5
Compare
|
This should now be green based on the previous TGP. Running a TAP Train to double-check before submitting. |
|
TAP Train was green, we're good to go! |
|
merge-assistance: this already got all the needed approvals and TAP Train was green. |
jessicajaniuk
left a comment
There was a problem hiding this comment.
reviewed-for: public-api
dylhunn
left a comment
There was a problem hiding this comment.
reviewed-for: public-api
|
This PR was merged into the repository by commit bc54687. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
…ameters` requirement
In AOT compilations, the
strictInjectionParameterscompiler option canbe enabled to report errors when an
@Injectableannotated class has aconstructor with parameters that do not provide an injection token, e.g.
only a primitive type or interface.
Since Ivy it's become required that any class with Angular behavior
(e.g. the
ngOnDestroylifecycle hook) is decorated using an Angulardecorator, which meant that
@Injectable()may need to have been addedto abstract base classes. Doing so would then report an error if
strictInjectionParametersis enabled, if the abstract class has anincompatible constructor for DI purposes. This may be fine though, as
a subclass may call the constructor explicitly without relying on
Angular's DI mechanism.
Therefore, this commit excludes abstract classes from the
strictInjectionParameterscheck. This avoids an error from beingreported at compile time. If the constructor ends up being used by
Angular's DI system at runtime, then the factory function of the
abstract class will throw an error by means of the
ɵɵinvalidFactoryinstruction.
BREAKING CHANGE: Invalid constructors for DI may now report compilation errors
When a class inherits its constructor from a base class, the compiler may now
report an error when that constructor cannot be used for DI purposes. This may
either be because the base class is missing an Angular decorator such as
@Injectable()or@Directive(), or because the constructor contains parameterswhich do not have an associated token (such as primitive types like
string).These situations used to behave unexpectedly at runtime, where the class may be
constructed without any of its constructor parameters, so this is now reported
as an error during compilation.
Any new errors that may be reported because of this change can be resolved either
by decorating the base class from which the constructor is inherited, or by adding
an explicit constructor to the class for which the error is reported.
Closes #37914