Skip to content

Conversation

hawkgs
Copy link
Member

@hawkgs hawkgs commented Sep 1, 2025

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)

What is the current behavior?

The TreeVisualizer is instantiated directly in the client components.

What is the new behavior?

Use the TreeVisualizer internally in the TreeVisualizerHost component instead of managing these separately in their client components (e.g. Injector and Router tree components/tabs).

@hawkgs hawkgs requested a review from dgp1130 September 1, 2025 14:41
@ngbot ngbot bot added this to the Backlog milestone Sep 1, 2025
@hawkgs hawkgs added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 1, 2025
@hawkgs hawkgs removed the request for review from dgp1130 September 1, 2025 15:10
@hawkgs hawkgs force-pushed the devtools/integrate-tree-vis-in-cmp branch from 954c1e7 to 2144618 Compare September 2, 2025 13:06
@hawkgs hawkgs requested a review from dgp1130 September 2, 2025 14:58
@hawkgs hawkgs removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 2, 2025
Copy link
Contributor

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No immediate concerns on my side, but I admit a lot of the router tree stuff goes over my head, so I'm not entirely following the change here. Maybe @AleksanderBodurri or @sumitarora could provide more meaningful feedback than I can?

@hawkgs
Copy link
Member Author

hawkgs commented Sep 3, 2025

@dgp1130, I hope that this timeline clarifies the purpose of this change:

  • Before Feb'25: The Injector and Router trees used entirely separate template, styles and business logic, while being almost identical.
  • Feb'25: TreeVisualizerHost is introduced – encapsulates the styles and the SVG container where the tree is being rendered refactor(devtools): introduce tree-visualizer-host #59916
  • Jun'25: The business logic for the tree rendering (D3) is extracted from both Injector and Router trees and moved to a new class called TreeVisualizer refactor(devtools): abstract and reuse the tree visualizer #62264
  • Now/This change: The business logic (i.e. TreeVisualizer class) is integrated directly into the TreeVisualizerHost component in order to drop all friction points where the user had to pass the SVG references from the host component to the tree visualizer class. In other words, now we have a single unit (the component) that is in charge or rendering the trees.

…izerHost component

Use the `TreeVisualizer` internally in the host component instead of managing these separately in their client components.
@hawkgs hawkgs force-pushed the devtools/integrate-tree-vis-in-cmp branch from 2144618 to 498fdbd Compare September 3, 2025 08:24
@hawkgs hawkgs added the target: minor This PR is targeted for the next minor release label Sep 3, 2025
@hawkgs hawkgs force-pushed the devtools/integrate-tree-vis-in-cmp branch from 652ddc1 to d1786f6 Compare September 3, 2025 08:48
Comment on lines -1 to -4
@if (routerDebugApiSupport()) {
<div class="filter">
<input
#searchInput
Copy link
Member Author

@hawkgs hawkgs Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AleksanderBodurri, would it make sense to you to disable the tab from here instead:

https://github.com/hawkgs/angular/blob/d1786f6b6a3284d58d942863f0399f04b6531351/devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.component.ts#L105

I assume this check was added to cover the case when the router tree tab has been enabled in a previous session, but we don't have routes to show in the current one, since the switch in the settings menu is already rendered only if supportedApis.routes is true.

@hawkgs hawkgs force-pushed the devtools/integrate-tree-vis-in-cmp branch from d1786f6 to 3608ae3 Compare September 3, 2025 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: devtools target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants