Skip to content

Commit d0cd74a

Browse files
sheikalthafthePunderWoman
sheikalthaf
authored and
thePunderWoman
committed
refactor(devtools): use signals for template properties in frame manager (angular#58818)
convert the frames and selectedFrame properties to signal so that it can react to changes on OnPush PR Close angular#58818
1 parent d298d25 commit d0cd74a

File tree

6 files changed

+71
-61
lines changed

6 files changed

+71
-61
lines changed

‎devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.component.html‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
class="frame-selector"
2525
(change)="emitSelectedFrame($event.target.value)"
2626
>
27-
@for (frame of frameManager.frames; track frame.id) {
27+
@for (frame of frameManager.frames(); track frame.id) {
2828
<option [value]="frame.id" [selected]="frameManager.isSelectedFrame(frame)">
2929
@if (frame.id === TOP_LEVEL_FRAME_ID) {
3030
top
@@ -69,7 +69,7 @@
6969
</nav>
7070

7171
<mat-tab-nav-panel #tabPanel>
72-
@if (!applicationEnvironment.frameSelectorEnabled || frameManager.selectedFrame !== null) {
72+
@if (!applicationEnvironment.frameSelectorEnabled || frameManager.selectedFrame() !== null) {
7373
<div class="tab-content">
7474
<ng-directive-explorer
7575
[showCommentNodes]="showCommentNodes()"

‎devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.component.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ export class DevToolsTabsComponent {
101101
}
102102

103103
emitSelectedFrame(frameId: string): void {
104-
const frame = this.frameManager.frames.find((frame) => frame.id === parseInt(frameId, 10));
104+
const frame = this.frameManager.frames().find((frame) => frame.id === parseInt(frameId, 10));
105105
this.frameSelected.emit(frame!);
106106
}
107107

‎devtools/projects/ng-devtools/src/lib/devtools-tabs/devtools-tabs.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,6 @@ describe('DevtoolsTabsComponent', () => {
9494
spyOn(comp.frameSelected, 'emit');
9595
comp.emitSelectedFrame('1');
9696

97-
expect(comp.frameSelected.emit).toHaveBeenCalledWith(comp.frameManager.frames[0]);
97+
expect(comp.frameSelected.emit).toHaveBeenCalledWith(comp.frameManager.frames()[0]);
9898
});
9999
});

‎devtools/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.component.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ export class DirectiveExplorerComponent {
191191
(directive) => directive.name === directiveName,
192192
);
193193

194-
const selectedFrame = this._frameManager.selectedFrame;
194+
const selectedFrame = this._frameManager.selectedFrame();
195195
if (!this._frameManager.frameHasUniqueUrl(selectedFrame)) {
196196
this._messageBus.emit('log', [
197197
{
@@ -210,7 +210,7 @@ export class DirectiveExplorerComponent {
210210
}
211211

212212
handleSelectDomElement(node: IndexedNode): void {
213-
const selectedFrame = this._frameManager.selectedFrame;
213+
const selectedFrame = this._frameManager.selectedFrame();
214214
if (!this._frameManager.frameHasUniqueUrl(selectedFrame)) {
215215
this._messageBus.emit('log', [
216216
{
@@ -290,7 +290,7 @@ export class DirectiveExplorerComponent {
290290
}): void {
291291
const objectPath = constructPathOfKeysToPropertyValue(node.prop);
292292

293-
const selectedFrame = this._frameManager.selectedFrame;
293+
const selectedFrame = this._frameManager.selectedFrame();
294294
if (!this._frameManager.frameHasUniqueUrl(selectedFrame)) {
295295
this._messageBus.emit('log', [
296296
{

‎devtools/projects/ng-devtools/src/lib/frame_manager.ts‎

Lines changed: 37 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -6,30 +6,29 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9-
import {Injectable, inject} from '@angular/core';
9+
import {Injectable, inject, signal, computed} from '@angular/core';
1010
import {Events, MessageBus} from 'protocol';
1111

1212
import {Frame, TOP_LEVEL_FRAME_ID} from './application-environment';
1313

1414
@Injectable()
1515
export class FrameManager {
16-
private _selectedFrameId: number | null = null;
17-
private _frames = new Map<number, Frame>();
16+
private _selectedFrameId = signal<number | null>(null);
17+
private _frames = signal(new Map<number, Frame>());
1818
private _inspectedWindowTabId: number | null = null;
1919
private _frameUrlToFrameIds = new Map<string, Set<number>>();
2020
private _messageBus = inject<MessageBus<Events>>(MessageBus);
2121

22-
get frames(): Frame[] {
23-
return Array.from(this._frames.values());
24-
}
22+
readonly frames = computed(() => Array.from(this._frames().values()));
2523

26-
get selectedFrame(): Frame | null {
27-
if (this._selectedFrameId === null) {
24+
readonly selectedFrame = computed(() => {
25+
const selectedFrameId = this._selectedFrameId();
26+
if (selectedFrameId === null) {
2827
return null;
2928
}
3029

31-
return this._frames.get(this._selectedFrameId) ?? null;
32-
}
30+
return this._frames().get(selectedFrameId) ?? null;
31+
});
3332

3433
static initialize(inspectedWindowTabIdTestOnly?: number | null) {
3534
const manager = new FrameManager();
@@ -45,8 +44,8 @@ export class FrameManager {
4544
}
4645

4746
this._messageBus.on('frameConnected', (frameId: number) => {
48-
if (this._frames.has(frameId)) {
49-
this._selectedFrameId = frameId;
47+
if (this._frames().has(frameId)) {
48+
this._selectedFrameId.set(frameId);
5049
}
5150
});
5251

@@ -58,48 +57,50 @@ export class FrameManager {
5857

5958
this.addFrame({name, id: frameId, url: urlWithoutHash});
6059

61-
if (this.frames.length === 1) {
62-
this.inspectFrame(this._frames.get(frameId)!);
60+
if (this.frames().length === 1) {
61+
this.inspectFrame(this._frames().get(frameId)!);
6362
}
6463
});
6564

6665
this._messageBus.on('contentScriptDisconnected', (frameId: number) => {
67-
if (!this._frames.has(frameId)) {
66+
const frame = this._frames().get(frameId);
67+
if (!frame) {
6868
return;
6969
}
7070

71-
this.removeFrame(this._frames.get(frameId)!);
71+
this.removeFrame(frame);
7272

7373
// Defensive check. This case should never happen, since we're always connected to at least
7474
// the top level frame.
75-
if (this.frames.length === 0) {
76-
this._selectedFrameId = null;
75+
if (this.frames().length === 0) {
76+
this._selectedFrameId.set(null);
7777
console.error('Angular DevTools is not connected to any frames.');
7878
return;
7979
}
8080

81-
if (frameId === this._selectedFrameId) {
82-
this._selectedFrameId = TOP_LEVEL_FRAME_ID;
83-
this.inspectFrame(this._frames.get(this._selectedFrameId!)!);
81+
const selectedFrameId = this._selectedFrameId();
82+
if (frameId === selectedFrameId) {
83+
this._selectedFrameId.set(TOP_LEVEL_FRAME_ID);
84+
this.inspectFrame(this._frames().get(TOP_LEVEL_FRAME_ID)!);
8485
return;
8586
}
8687
});
8788
}
8889

8990
isSelectedFrame(frame: Frame): boolean {
90-
return this._selectedFrameId === frame.id;
91+
return this._selectedFrameId() === frame.id;
9192
}
9293

9394
inspectFrame(frame: Frame): void {
9495
if (this._inspectedWindowTabId === null) {
9596
return;
9697
}
9798

98-
if (!this._frames.has(frame.id)) {
99+
if (!this._frames().has(frame.id)) {
99100
throw new Error('Attempted to inspect a frame that is not connected to Angular DevTools.');
100101
}
101102

102-
this._selectedFrameId = null;
103+
this._selectedFrameId.set(null);
103104
this._messageBus.emit('enableFrameConnection', [frame.id, this._inspectedWindowTabId]);
104105
}
105106

@@ -113,11 +114,14 @@ export class FrameManager {
113114
}
114115

115116
private addFrame(frame: Frame): void {
116-
this._frames.set(frame.id, frame);
117-
const frameUrl = frame.url.toString();
118-
const frameIdSet = this._frameUrlToFrameIds.get(frameUrl) ?? new Set<number>();
119-
frameIdSet.add(frame.id);
120-
this._frameUrlToFrameIds.set(frameUrl, frameIdSet);
117+
this._frames.update((frames) => {
118+
frames.set(frame.id, frame);
119+
const frameUrl = frame.url.toString();
120+
const frameIdSet = this._frameUrlToFrameIds.get(frameUrl) ?? new Set<number>();
121+
frameIdSet.add(frame.id);
122+
this._frameUrlToFrameIds.set(frameUrl, frameIdSet);
123+
return new Map(frames);
124+
});
121125
}
122126

123127
private removeFrame(frame: Frame): void {
@@ -128,6 +132,9 @@ export class FrameManager {
128132
if (urlFrameIds.size === 0) {
129133
this._frameUrlToFrameIds.delete(frameUrl);
130134
}
131-
this._frames.delete(frameId);
135+
this._frames.update((frames) => {
136+
frames.delete(frameId);
137+
return new Map(frames);
138+
});
132139
}
133140
}

‎devtools/projects/ng-devtools/src/lib/frame_manager_spec.ts‎

Lines changed: 27 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ describe('FrameManager', () => {
1717
let topicToCallback: {[topic: string]: Function | null};
1818

1919
function getFrameFromFrameManager(frameId: number): Frame | undefined {
20-
return frameManager.frames.find((f: Frame) => f.id === frameId);
20+
return frameManager.frames().find((f: Frame) => f.id === frameId);
2121
}
2222

2323
function frameConnected(frameId: number): void {
@@ -66,54 +66,55 @@ describe('FrameManager', () => {
6666

6767
it('should add frame when contentScriptConnected event is emitted', () => {
6868
contentScriptConnected(topLevelFrameId, 'name', 'http://localhost:4200/url');
69-
expect(frameManager.frames.length).toBe(1);
70-
expect(frameManager.frames[0].id).toBe(topLevelFrameId);
71-
expect(frameManager.frames[0].name).toBe('name');
72-
expect(frameManager.frames[0].url.toString()).toBe('http://localhost:4200/url');
69+
const frames = frameManager.frames();
70+
expect(frames.length).toBe(1);
71+
expect(frames[0].id).toBe(topLevelFrameId);
72+
expect(frames[0].name).toBe('name');
73+
expect(frames[0].url.toString()).toBe('http://localhost:4200/url');
7374
});
7475

7576
it('should set the selected frame to the first frame when there is only one frame', () => {
7677
contentScriptConnected(topLevelFrameId, 'name', 'http://localhost:4200/url');
77-
expect(frameManager.selectedFrame?.id).toBe(topLevelFrameId);
78+
expect(frameManager.selectedFrame()?.id).toBe(topLevelFrameId);
7879
});
7980

8081
it('should set selected frame when frameConnected event is emitted', () => {
8182
contentScriptConnected(topLevelFrameId, 'name', 'http://localhost:4200/url');
8283
contentScriptConnected(otherFrameId, 'name2', 'http://localhost:4200/url2');
8384
frameConnected(otherFrameId);
84-
expect(frameManager.selectedFrame?.id).toBe(otherFrameId);
85+
expect(frameManager.selectedFrame()?.id).toBe(otherFrameId);
8586
});
8687

8788
it('should remove frame when contentScriptDisconnected event is emitted', () => {
8889
contentScriptConnected(topLevelFrameId, 'name', 'http://localhost:4200/url');
8990
contentScriptConnected(otherFrameId, 'name2', 'http://localhost:4200/url2');
90-
expect(frameManager.frames.length).toBe(2);
91+
expect(frameManager.frames().length).toBe(2);
9192
contentScriptDisconnected(otherFrameId);
92-
expect(frameManager.frames.length).toBe(1);
93-
expect(frameManager.frames[0].id).toBe(topLevelFrameId);
93+
expect(frameManager.frames().length).toBe(1);
94+
expect(frameManager.frames()[0].id).toBe(topLevelFrameId);
9495

9596
const errorSpy = spyOn(console, 'error');
9697
contentScriptDisconnected(topLevelFrameId);
97-
expect(frameManager.frames.length).toBe(0);
98+
expect(frameManager.frames().length).toBe(0);
9899
expect(errorSpy).toHaveBeenCalledWith('Angular DevTools is not connected to any frames.');
99100
});
100101

101102
it('should set selected frame to top level frame when contentScriptDisconnected event is emitted for selected frame', () => {
102103
contentScriptConnected(topLevelFrameId, 'name', 'http://localhost:4200/url');
103104
contentScriptConnected(otherFrameId, 'name2', 'http://localhost:4200/url2');
104105
frameConnected(otherFrameId);
105-
expect(frameManager.selectedFrame?.id).toBe(otherFrameId);
106+
expect(frameManager.selectedFrame()?.id).toBe(otherFrameId);
106107
contentScriptDisconnected(otherFrameId);
107-
expect(frameManager.selectedFrame?.id).toBe(topLevelFrameId);
108+
expect(frameManager.selectedFrame()?.id).toBe(topLevelFrameId);
108109
});
109110

110111
it('should not set selected frame to top level frame when contentScriptDisconnected event is emitted for non selected frame', () => {
111112
contentScriptConnected(topLevelFrameId, 'name', 'http://localhost:4200/url');
112113
contentScriptConnected(otherFrameId, 'name2', 'http://localhost:4200/url2');
113114
frameConnected(topLevelFrameId);
114-
expect(frameManager.selectedFrame?.id).toBe(topLevelFrameId);
115+
expect(frameManager.selectedFrame()?.id).toBe(topLevelFrameId);
115116
contentScriptDisconnected(otherFrameId);
116-
expect(frameManager.selectedFrame?.id).toBe(topLevelFrameId);
117+
expect(frameManager.selectedFrame()?.id).toBe(topLevelFrameId);
117118
});
118119

119120
it('should not set selected frame to top level frame when contentScriptDisconnected event is emitted for non existing frame', () => {
@@ -122,9 +123,9 @@ describe('FrameManager', () => {
122123
contentScriptConnected(topLevelFrameId, 'name', 'http://localhost:4200/url');
123124
contentScriptConnected(otherFrameId, 'name2', 'http://localhost:4200/url2');
124125
frameConnected(otherFrameId);
125-
expect(frameManager.selectedFrame?.id).toBe(otherFrameId);
126+
expect(frameManager.selectedFrame()?.id).toBe(otherFrameId);
126127
contentScriptDisconnected(nonExistingFrameId);
127-
expect(frameManager.selectedFrame?.id).toBe(otherFrameId);
128+
expect(frameManager.selectedFrame()?.id).toBe(otherFrameId);
128129
});
129130

130131
it('isSelectedFrame should return true when frame matches selected frame', () => {
@@ -161,21 +162,21 @@ describe('FrameManager', () => {
161162
const topLevelFrame = getFrameFromFrameManager(topLevelFrameId);
162163
expect(topLevelFrame).toBeDefined();
163164
frameManager.inspectFrame(topLevelFrame!);
164-
expect(frameManager.selectedFrame?.id).toBe(topLevelFrameId);
165+
expect(frameManager.selectedFrame()?.id).toBe(topLevelFrameId);
165166
});
166167

167168
it('frameHasUniqueUrl should return false when a two frames have the same url', () => {
168169
contentScriptConnected(topLevelFrameId, 'name', 'https://angular.dev/');
169170
contentScriptConnected(otherFrameId, 'name2', 'https://angular.dev/');
170-
expect(frameManager.selectedFrame?.url.toString()).toBe('https://angular.dev/');
171-
expect(frameManager.frameHasUniqueUrl(frameManager.selectedFrame!)).toBe(false);
171+
expect(frameManager.selectedFrame()?.url.toString()).toBe('https://angular.dev/');
172+
expect(frameManager.frameHasUniqueUrl(frameManager.selectedFrame()!)).toBe(false);
172173
});
173174

174175
it('frameHasUniqueUrl should return true when only one frame has a given url', () => {
175176
contentScriptConnected(topLevelFrameId, 'name', 'https://angular.dev/');
176177
contentScriptConnected(otherFrameId, 'name', 'https://angular.dev/overview');
177-
expect(frameManager.selectedFrame?.url.toString()).toBe('https://angular.dev/');
178-
expect(frameManager.frameHasUniqueUrl(frameManager.selectedFrame!)).toBe(true);
178+
expect(frameManager.selectedFrame()?.url.toString()).toBe('https://angular.dev/');
179+
expect(frameManager.frameHasUniqueUrl(frameManager.selectedFrame()!)).toBe(true);
179180
});
180181

181182
it('frameHasUniqueUrl should not consider url fragments as part of the url comparison', () => {
@@ -185,8 +186,10 @@ describe('FrameManager', () => {
185186
'name',
186187
'https://angular.dev/guide/components#using-components',
187188
);
188-
expect(frameManager.selectedFrame?.url.toString()).toBe('https://angular.dev/guide/components');
189-
expect(frameManager.frameHasUniqueUrl(frameManager.selectedFrame!)).toBe(false);
189+
expect(frameManager.selectedFrame()?.url.toString()).toBe(
190+
'https://angular.dev/guide/components',
191+
);
192+
expect(frameManager.frameHasUniqueUrl(frameManager.selectedFrame()!)).toBe(false);
190193
});
191194

192195
it('frameHasUniqueUrl should return false when frame is null', () => {

0 commit comments

Comments
 (0)