fix(compiler-cli): compile dep without Array when useFactory in @Inje… #42994
Conversation
7f6c69b to
e51b927
Compare
…parameters (angular#42987) if node is not decorator node ,then think maybe is a token node.
fcd8197 to
c5e9d0a
Compare
|
FYI, adding @alxhub to this thread, since he helped with initial investigation, see #42987 (comment). |
| @@ -43,6 +45,46 @@ runInEachFileSystem(() => { | ||
| expect(res).not.toContain(jasmine.objectContaining({name: 'ɵprov'})); | ||
| }); | ||
| }); | ||
|
|
||
| describe('fix', () => { |
There was a problem hiding this comment.
In the compiler we typically have input->output tests, as they are usually better readable and provide a stronger guarantee that programs are compiled as expected. These can either live in packages/compiler-cli/test/ngtsc/ngtsc_spec.ts or as compliance test in packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_di/di. The compliance tests are a bit harder to get started with, but having one for this scenario would make sense to me. Please let me know if you need help with this.
| maybeUpdateDecorator(el.expression, reflector, token); | ||
| isDecorator = maybeUpdateDecorator(el.expression, reflector, token); | ||
| } | ||
| if (!isDecorator) { |
There was a problem hiding this comment.
I notice this logic is greedily accepting the last regular element as token, which seems really loose to me. However the runtime implements the exact same logic, so let's keep as is.
There was a problem hiding this comment.
Before I change this part of logic, when want to add parameter decorator, token must use new Inject(xxx) to mark.
Now when the item in array is not parameter decorator(new instance or class), while think this item is a token(token can be any type without parameter decorator)
There was a problem hiding this comment.
Indeed. It's loose in the sense that e.g. [A, B, C] would be interpreted as C and e.g. [new Optional(), new Inject(B), C] also as C, which should arguably report a diagnostic that something is wrong. As part of this bug fix however I think the current approach is fine, especially since it mirrors the runtime implementation.
There was a problem hiding this comment.
But the logic is similar with
angular/packages/core/src/di/injector_compatibility.ts
Lines 138 to 169 in 7846715
In line 160,If is not decorator, guess is a token
There was a problem hiding this comment.
That's correct and what I meant by "I think the current approach is fine, especially since it mirrors the runtime implementation."
Still, I would like to see this change tested in integration-like tests as commented here.
|
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. |
…ctable
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #42987
What is the new behavior?
Does this PR introduce a breaking change?
Other information