fix(compiler-cli): correctly interpret token arrays in @Injectable deps
#43226
Conversation
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
|
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 |
|
@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 |
|
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 |
|
@googlebot I consent. |
I kind of feel that we should be less relaxed about the ordering and duplication of tokens in such an array. If not an error then a warning, since it is unlikely that the developer meant to put more than one token into the array.
Apart from that this LGTM.
| if (!isDecorator) { | ||
| meta.token = new WrappedNodeExpr(el); |
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?
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(), |
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), |
Changed.
|
@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
I share your feeling that duplication of tokens should be reported as an error (see #42994 (comment) |
…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
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.
The text was updated successfully, but these errors were encountered: