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
DevTools DI Visualization Features #51719
DevTools DI Visualization Features #51719
Conversation
9c92609
to
ee0cff2
Compare
any screenshot? |
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.
Thanks for putting this all together! Definitely a lot of d3 complexity here. I'm only about halfway through right now. Most of it are nitpicks / random code quality suggestions. Feel free to push back on any of these, I don't think any are blocking this PR.
The biggest question I have is what the impact of the @angular/core
fix is on the versioning of the release. Does this mean we need the latest patch version to do any DI debugging? Is it fixing a specific bug in a specific situation or is this more load-bearing?
I would love to get a review from someone who understands more about DI. I can't provide a whole lot of feedback outside of general observations. Maybe Andrew Kushnir could take a look whenever you think it's ready?
.../projects/ng-devtools/src/lib/devtools-tabs/dependency-injection/injector-tree-visualizer.ts
Outdated
Show resolved
Hide resolved
(d: any): | ||
any => { | ||
if (this._orientation === 'horizontal') { | ||
if (!d.parent && !d.data?.children?.length) { |
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.
Consider: Are there any small utilities we could come up with to reduce the boilerplate and help communicate the intent of this code? d.parent
and !d.parent
are probably straightforward enough, but I wonder if d.data?.children?.length
could be called hasChildren(d)
or something like that? This would similarly help reduce the noise and draw more attention to a leading !
which is the important part of those expressions.
One other approach to consider (I don't really know if this makes much sense) is to better define the names of each of these cases like:
node.append('text').attr('dx', (d: any) => getDelta(d, {
noParentNoChildren: 15,
noParentHasChildren: 8,
hasParentNoChildren: 15,
hasParentHasChildren: -15,
}));
.../projects/ng-devtools/src/lib/devtools-tabs/dependency-injection/injector-tree-visualizer.ts
Outdated
Show resolved
Hide resolved
.../projects/ng-devtools/src/lib/devtools-tabs/dependency-injection/injector-tree-visualizer.ts
Outdated
Show resolved
Hide resolved
.../projects/ng-devtools/src/lib/devtools-tabs/dependency-injection/injector-tree-visualizer.ts
Outdated
Show resolved
Hide resolved
ee0cff2
to
5f65c8d
Compare
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.
Ok, finally got through the rest of it. Again, it's mostly pretty minor nitpicks and quality suggestions, nothing very critical. A couple comments are probably related to the fact that the PR is still in draft state and not fully complete.
...projects/ng-devtools/src/lib/devtools-tabs/dependency-injection/resolution-path.component.ts
Outdated
Show resolved
Hide resolved
...projects/ng-devtools/src/lib/devtools-tabs/dependency-injection/resolution-path.component.ts
Outdated
Show resolved
Hide resolved
...projects/ng-devtools/src/lib/devtools-tabs/dependency-injection/resolution-path.component.ts
Outdated
Show resolved
Hide resolved
...projects/ng-devtools/src/lib/devtools-tabs/dependency-injection/resolution-path.component.ts
Outdated
Show resolved
Hide resolved
...projects/ng-devtools/src/lib/devtools-tabs/dependency-injection/resolution-path.component.ts
Show resolved
Hide resolved
font-weight: 300; | ||
} | ||
} | ||
} |
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.
Nit: Indentation. We're inconsistently using 2 or 4 spaces.
devtools/projects/ng-devtools/src/lib/devtools-tabs/injector-tree/injector-tree.component.scss
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
:host { |
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.
Suggestion: This is a big file and selectors like :host
and ::ng-deep
seem to repeat a lot. I suggest either grouping all the common selectors together or sectioning them into related styles. For example, .deps
, .no-deps
, :host-context(.dark-theme) { .deps { } }
can be reasonably grouped together, even if other dark theme stuff exists elsewhere.
Actually the biggest thing is probably the lack of tests. Testing the d3 stuff might be difficult and arguably not worth it, but we could certainly test a lot of the injection tree transformations and component behavior outside of rendering. |
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.
reviewed-for: fw-core
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.
Everything looks promising, so I'm very excited to this go in! I'll approve now just so this isn't blocked while I'm OOO, but take your time on the implementation. Definitely looking forward to seeing this land!
ce33cae
to
a542baa
Compare
@AleksanderBodurri Looks like this still has some CI issues. |
a542baa
to
176fd03
Compare
Will do a bit more cleanup here related to @dgp1130 's feedback before we merge |
97a87d7
to
477b6ea
Compare
@AleksanderBodurri it looks like there is a legit error in core acceptance tests, see this CI job output. Could you please take a look when you get a chance? |
Update: this PR needs cleanup: there is a legit case were |
e19afc5
to
d114f27
Compare
… getInjectorMetadata Previously getDependenciesForTokenInInjector was unable to determine which node on a view serviced a specific injection. Now it is able to filter out those injections that did not come from the specific node for the NodeInjector passed into it. Previously getInjectorMetadata was incorrectly looking up DOM elements for some directives (for example NgForOf) where an LContainer was created. Now the LContainer case is handled, and the non LContainer case uses `getFirstNativeNode` to more accurately get the element we want.
This commit introduces 2 new features into DevTools. Directive level dependency inspection: Users can now view which dependencies their directives have injected in the property viewer tab. This view displays not only the dependency but also the resolution path that was used to service the injection. Injector graph inspection: Users can now view a visualization of the element and environment hierarchies in their application. These trees are displayed separately but on the same page in the Injector Tree tab. User can click on individual injectors to view a list of all the providers configured in that injector, as well as highlight the resolution path from that injector to the root (with the corresponding environment injector connection highlighted as well).
…nctions Creates set of unit tests for each function in the data transformation pipeline that enables injector metadata to be visualized as d3 graphs.
d114f27
to
85374f7
Compare
Caretaker note: presubmit and CI is "green", this PR is ready for merge. |
This PR was merged into the repository by commit 8b91864. |
…51719) This commit introduces 2 new features into DevTools. Directive level dependency inspection: Users can now view which dependencies their directives have injected in the property viewer tab. This view displays not only the dependency but also the resolution path that was used to service the injection. Injector graph inspection: Users can now view a visualization of the element and environment hierarchies in their application. These trees are displayed separately but on the same page in the Injector Tree tab. User can click on individual injectors to view a list of all the providers configured in that injector, as well as highlight the resolution path from that injector to the root (with the corresponding environment injector connection highlighted as well). PR Close #51719
Hello. Thanks a lot for this contribution! I would really like to test this on our project, as it might help us a lot for our refactoring. Thanks a lot! |
The sources for the extensions are available on the repo. Here are the build instructions : https://github.com/angular/angular/blob/main/devtools/README.md |
@haraldreingruber-dedalus just FYI, this will be published alongside Angular v17 very shortly: https://angular.io/guide/releases#support-policy-and-schedule Also this particular feature requires your app to be run on v17, so you'll need to upgrade first. You can test out We'd definitely like to hear any feedback you have from trying it, I'm just letting you know what you're in for if you try to use this early. |
@AleksanderBodurri @dgp1130 Should I expect to see all entities of the DI container? Or maybe many important entities are missing because our project is split into several libraries? Still, I think this feature is great for debugging and refactoring :) |
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. |
… getInjectorMetadata (angular#51719) Previously getDependenciesForTokenInInjector was unable to determine which node on a view serviced a specific injection. Now it is able to filter out those injections that did not come from the specific node for the NodeInjector passed into it. Previously getInjectorMetadata was incorrectly looking up DOM elements for some directives (for example NgForOf) where an LContainer was created. Now the LContainer case is handled, and the non LContainer case uses `getFirstNativeNode` to more accurately get the element we want. PR Close angular#51719
…ngular#51719) This commit introduces 2 new features into DevTools. Directive level dependency inspection: Users can now view which dependencies their directives have injected in the property viewer tab. This view displays not only the dependency but also the resolution path that was used to service the injection. Injector graph inspection: Users can now view a visualization of the element and environment hierarchies in their application. These trees are displayed separately but on the same page in the Injector Tree tab. User can click on individual injectors to view a list of all the providers configured in that injector, as well as highlight the resolution path from that injector to the root (with the corresponding environment injector connection highlighted as well). PR Close angular#51719
…nctions (angular#51719) Creates set of unit tests for each function in the data transformation pipeline that enables injector metadata to be visualized as d3 graphs. PR Close angular#51719
See individual commits.
Note that the majority of the LOC additions come from mock data in unit tests.