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): allow abstract directives to have an invalid constructor #32987
Conversation
@@ -133,6 +133,21 @@ export function valueReferenceToExpression( | |||
} | |||
} | |||
|
|||
export function unwrapConstructorDependencies(deps: ConstructorDeps | null): R3DependencyMetadata[]| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, this LGTM :) I just changed the comments.
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. |
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). |
3ddfa57
to
b9c1c31
Compare
We came up with an idea to generate |
b9c1c31
to
75bc9d7
Compare
75bc9d7
to
341fe74
Compare
|
||
describe('abstract directives', () => { | ||
it('should generate a factory function that throws', () => { | ||
env.tsconfig({strictInjectionParameters: false}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added :)
export interface R3FactoryDefMetadata { | ||
name: string; | ||
type: o.Expression; | ||
typeArgumentCount: number; | ||
deps: R3DependencyMetadata[]|'invalid'|null; | ||
injectFn: o.ExternalReference; | ||
isPipe?: boolean; | ||
target: R3FactoryTarget; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. JIT directive compilation was adding a factory intended for pipes, which uses a different ChangeDetectorRef
implementation.
341fe74
to
ee02310
Compare
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
ee02310
to
fc8481b
Compare
Caretaker: public API is unaffected (addition of a private API) |
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. |
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 explicituse*
definition allows for the constructor to be irrelevant. Forexample, the situation where
useFactory
is specified allows for theconstructor to be called explicitly with any value, so its constructor
parameters are not required to be valid. For
@Injectable
s this ishandled 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
@Injectable