refactor(core): include DI path into cyclic dependency error message#50902
Closed
JeanMeche wants to merge 7 commits intoangular:mainfrom
Closed
refactor(core): include DI path into cyclic dependency error message#50902JeanMeche wants to merge 7 commits intoangular:mainfrom
JeanMeche wants to merge 7 commits intoangular:mainfrom
Conversation
fb181b7 to
17ac2cc
Compare
3 tasks
17ac2cc to
e879e63
Compare
JeanMeche
commented
Jul 1, 2023
packages/core/src/render3/di.ts
Outdated
Member
Author
There was a problem hiding this comment.
Is there a better way to retrieve the name of the class ?
Member
There was a problem hiding this comment.
This is also a bit sensitive to the compiler output. It looks like we construct the NodeInjectorFactory in these places:
- In
configureViewWithDirectivewe should be able to capture the name from the directive def. - The two calls in
resolveProvidermight have access to it through the DI token. It's not guaranteed, but we can fall back to something like<unknown>or<anonymous>.
Member
Author
There was a problem hiding this comment.
Are you suggesting something like ?
if(factory.factory.name) {
name = factory.factory.name.slice(0, -8)
} else {
name = 'unknown'
}
Member
There was a problem hiding this comment.
No, I mean that we try to capture the name when constructing the NodeInjectorFactory and then fall back to something like unknown.
e879e63 to
710947e
Compare
00468eb to
539fbb2
Compare
Contributor
|
@JeanMeche could you please rebase this PR when you get a chance? We'll perform code review and see if we can land it in v17. Thank you. |
539fbb2 to
9649b7a
Compare
Contributor
9649b7a to
ab10a83
Compare
0cafc1f to
9798e85
Compare
crisbeto
reviewed
Jul 4, 2025
packages/core/src/render3/di.ts
Outdated
Member
There was a problem hiding this comment.
This is also a bit sensitive to the compiler output. It looks like we construct the NodeInjectorFactory in these places:
- In
configureViewWithDirectivewe should be able to capture the name from the directive def. - The two calls in
resolveProvidermight have access to it through the DI token. It's not guaranteed, but we can fall back to something like<unknown>or<anonymous>.
9798e85 to
08efa45
Compare
This commit updates the logic to better handle a situation when there is a cyclic DI dependency detected. Previously, the error message used to contain the name of the token that triggered the problem. With this change, the DI resolution path would also be included, so that it's easier to find and resolve the cycle.
08efa45 to
9432d01
Compare
7fd3fbe to
176e099
Compare
crisbeto
approved these changes
Jul 6, 2025
Member
Author
|
TESTED=TGP |
Contributor
|
This PR was merged into the repository by commit 96014b5. The changes were merged into the following branches: main, 20.1.x |
AndrewKushnir
added a commit
that referenced
this pull request
Jul 10, 2025
…50902) This commit updates the logic to better handle a situation when there is a cyclic DI dependency detected. Previously, the error message used to contain the name of the token that triggered the problem. With this change, the DI resolution path would also be included, so that it's easier to find and resolve the cycle. PR Close #50902
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit updates the logic to better handle a situation when there is a cyclic DI dependency detected. Previously, the error message used to contain the name of the token that triggered the problem. With this change, the DI resolution path would also be included, so that it's easier to find and resolve the cycle.
I'd like to credit @AndrewKushnir who did most of the work on this.
Fixes: #46189