feat(devtools): create devtools signals view #61919
Conversation
8a32ab1 to
66ebbb2
Compare
...ects/ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signal-data-source.ts
Outdated
Show resolved
Hide resolved
...ects/ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signal-data-source.ts
Outdated
Show resolved
Hide resolved
...ects/ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signal-data-source.ts
Outdated
Show resolved
Hide resolved
...ects/ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signal-data-source.ts
Outdated
Show resolved
Hide resolved
...s/ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signals-tab.component.ts
Outdated
Show resolved
Hide resolved
...s/ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signals-tab.component.ts
Outdated
Show resolved
Hide resolved
| if (descriptor.type === PropType.Object || descriptor.type === PropType.Array) { | ||
| return arrayifyProps(descriptor.value || {}, prop); | ||
| } | ||
| return; |
There was a problem hiding this comment.
Should be redundant since the fn will return undefined anyway.
There was a problem hiding this comment.
This helps satisfy the "not all code paths return a value" lint because the return type is undefined instead of void
There was a problem hiding this comment.
Hmm, yeah, it's a warning. I thought the compiler is handling that but I guess it's because of the "noImplicitReturns": true. Okay 👍
...ects/ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signals-visualizer.ts
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| const createNode = (node: DebugSignalGraphNode) => { | ||
| const outer = document.createElement('div'); |
There was a problem hiding this comment.
Can we use the Renderer2 for the element creation (or DOCUMENT) and manipulation? I guess you'll have to pass an Injector or the renderer itself via the constructor.
I know there are other places that directly rely on document but we should try to limit these instances.
There was a problem hiding this comment.
Switching to DOCUMENT won't help much because d3 uses document internally. Rendering an angular template here would be ideal, but maybe that makes sense to do in a follow up PR?
There was a problem hiding this comment.
Yeah, I thought about that. It's more about testing and being able to mock the calls, if required, rather than keeping the code platform agnostic (D3 is already violating that as you noted) but, on second though, unit testing this class might be just too time consuming compared to an E2E that'll provide more value. I guess you can leave it as is 👍.
...ects/ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signals-visualizer.ts
Show resolved
Hide resolved
...ects/ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signals-visualizer.ts
Outdated
Show resolved
Hide resolved
...ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signals-tab.component.html
Outdated
Show resolved
Hide resolved
...ects/ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signals-visualizer.ts
Show resolved
Hide resolved
...ects/ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signal-data-source.ts
Outdated
Show resolved
Hide resolved
|
Looks good! ✨ Left minor/style comments. I'll let the rest of the team to chime in. I guess this shouldn't be a blocker for moving the PR forward but can we test some of the code? I realize that it's tricky to do that for complex UI without getting into E2E but maybe we can do some unit testing of the more frictionless parts. |
...ects/ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signals-visualizer.ts
Outdated
Show resolved
Hide resolved
...ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signals-tab.component.html
Outdated
Show resolved
Hide resolved
...ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signals-tab.component.html
Outdated
Show resolved
Hide resolved
...s/ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signals-tab.component.ts
Outdated
Show resolved
Hide resolved
...ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signals-tab.component.html
Outdated
Show resolved
Hide resolved
create a component for the devtools signal pane, including view source, view value, and flashing of node updates
| const parsedB = parseInt(b.name, 10); | ||
|
|
||
| if (isNaN(parsedA) || isNaN(parsedB)) { | ||
| return a.name > b.name ? 1 : -1; |
There was a problem hiding this comment.
Should we do a localeCompare here instead ?
| this.expandedData.next( | ||
| this.treeFlattener.expandFlattenedNodes(this.data.value, this.treeControl), | ||
| ); |
There was a problem hiding this comment.
This is a side effect in a map.
| private data = new BehaviorSubject<FlatNode[]>([]); | ||
| private expandedData = new BehaviorSubject<FlatNode[]>([]); |
There was a problem hiding this comment.
Do we realy need a stream here ?
I was under the impression that a signal would fit here.
create a component for the devtools signal pane, including view source, view value, and flashing of node updates
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: N/A
What is the new behavior?
Blocked on #61917, #61918See #60478 for a more complete demo
Does this PR introduce a breaking change?
Other information