fix(compiler-cli): correctly interpret token arrays in @Injectable deps
#43226
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@wszgrcy I have added some fixup commits to your commit to get your contribution landed, but would need your explicit consent to get the google-cla bot happy. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
78f9ca7 to
ca8e65f
Compare
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
| if (!isDecorator) { | ||
| meta.token = new WrappedNodeExpr(el); |
There was a problem hiding this comment.
This means that the array could be of the form:
['a', new Optional()]
Is this desirable? Or should we require that the token comes after any decorators?
There was a problem hiding this comment.
This implements the same logic as the runtime; that doesn't necessarily mean that it's desirable but I wouldn't see a reason to disallow this.
|
|
||
| @Injectable({ | ||
| providedIn: 'root', | ||
| useFactory: () => new MyAlternateService(), |
There was a problem hiding this comment.
Although this is correct it doesn't really show the intention of using a dependency in the factory.
Perhaps this should be:
| useFactory: () => new MyAlternateService(), | |
| useFactory: (dep: SomeDep|null) => new MyAlternateService(dep), |
|
@JoostK could you please rebase this PR when you get a chance? It looks like it was rebased a while ago and I can not start a presubmit because of that. Thank you. |
…eps` When specifying the `deps` array in the `@Injectable` decorator to inject dependencies into the injectable's factory function, it should be possible to use an array literal to configure how the dependency should be resolved by the DI system. For example, the following example is allowed: ```ts @Injectable({ providedIn: 'root', useFactory: a => new AppService(a), deps: [[new Optional(), 'a']], }) export class AppService { constructor(a) {} } ``` Here, the `'a'` string token should be injected as optional. However, the AOT compiler incorrectly used the array literal itself as injection token, resulting in a failure at runtime. Only if the token were to be provided using `[new Optional(), new Inject('a')]` would it work correctly. This commit fixes the issue by using the last non-decorator in the array literal as the token value, instead of the array literal itself. Note that this is a loose interpretation of array literals: if a token is omitted from the array literal then the array literal itself is used as token, but any decorator such as `new Optional()` would still have been applied. When there's multiple tokens in the list then only the last one will be used as actual token, any prior tokens are silently ignored. This behavior mirrors the JIT interpretation so is kept as is for now, but may benefit from some stricter checking and better error reporting in the future. Fixes angular#42987
ca8e65f to
6fec133
Compare
I share your feeling that duplication of tokens should be reported as an error (see #42994 (comment) 😄), but strictly speaking that would be a breaking change and separate from this fix. |
…eps` (#43226) When specifying the `deps` array in the `@Injectable` decorator to inject dependencies into the injectable's factory function, it should be possible to use an array literal to configure how the dependency should be resolved by the DI system. For example, the following example is allowed: ```ts @Injectable({ providedIn: 'root', useFactory: a => new AppService(a), deps: [[new Optional(), 'a']], }) export class AppService { constructor(a) {} } ``` Here, the `'a'` string token should be injected as optional. However, the AOT compiler incorrectly used the array literal itself as injection token, resulting in a failure at runtime. Only if the token were to be provided using `[new Optional(), new Inject('a')]` would it work correctly. This commit fixes the issue by using the last non-decorator in the array literal as the token value, instead of the array literal itself. Note that this is a loose interpretation of array literals: if a token is omitted from the array literal then the array literal itself is used as token, but any decorator such as `new Optional()` would still have been applied. When there's multiple tokens in the list then only the last one will be used as actual token, any prior tokens are silently ignored. This behavior mirrors the JIT interpretation so is kept as is for now, but may benefit from some stricter checking and better error reporting in the future. Fixes #42987 PR Close #43226
|
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. |
When specifying the
depsarray in the@Injectabledecorator toinject dependencies into the injectable's factory function, it should
be possible to use an array literal to configure how the dependency
should be resolved by the DI system.
For example, the following example is allowed:
Here, the
'a'string token should be injected as optional. However,the AOT compiler incorrectly used the array literal itself as injection
token, resulting in a failure at runtime. Only if the token were to be
provided using
[new Optional(), new Inject('a')]would it workcorrectly.
This commit fixes the issue by using the last non-decorator in the
array literal as the token value, instead of the array literal itself.
Note that this is a loose interpretation of array literals: if a token
is omitted from the array literal then the array literal itself is used
as token, but any decorator such as
new Optional()would still havebeen applied. When there's multiple tokens in the list then only the
last one will be used as actual token, any prior tokens are silently
ignored. This behavior mirrors the JIT interpretation so is kept as is
for now, but may benefit from some stricter checking and better error
reporting in the future.
Fixes #42987
Thanks to @wszgrcy for their contribution.