From b127cdc211fe7a73b831a3f1561e0e90ab36181a Mon Sep 17 00:00:00 2001 From: mgechev Date: Fri, 20 Mar 2020 17:19:27 -0700 Subject: [PATCH] fix(devtools): prevent race condition between getting nested props and refresh --- .../src/lib/component-tree.ts | 2 +- .../directive-explorer.component.ts | 28 ++++++++++--- .../directive-explorer.spec.ts | 27 ++++++------ .../directive-property-resolver.ts | 8 +++- .../element-property-resolver.spec.ts | 42 ++++++++++++------- .../element-property-resolver.ts | 9 +++- .../property-resolver/property-data-source.ts | 8 +++- 7 files changed, 83 insertions(+), 41 deletions(-) diff --git a/projects/ng-devtools-backend/src/lib/component-tree.ts b/projects/ng-devtools-backend/src/lib/component-tree.ts index 81b50f0847dd6..1efcc7965ee1b 100644 --- a/projects/ng-devtools-backend/src/lib/component-tree.ts +++ b/projects/ng-devtools-backend/src/lib/component-tree.ts @@ -45,7 +45,7 @@ export const getLatestComponentState = (query: ComponentExplorerViewQuery): Dire } if (query.propertyQuery.type === PropertyQueryTypes.Specified) { result[dir.name] = { - props: deeplySerializeSelectedProperties(dir.instance, query.propertyQuery.properties[dir.name]), + props: deeplySerializeSelectedProperties(dir.instance, query.propertyQuery.properties[dir.name] || []), }; } }; diff --git a/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.component.ts b/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.component.ts index 2f7d613d4027f..4fbe4303221b0 100644 --- a/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.component.ts +++ b/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.component.ts @@ -55,6 +55,8 @@ export class DirectiveExplorerComponent implements OnInit { private _changeSize = new Subject(); private _clickedElement: IndexedNode | null = null; + private _requestingNestedProperties = false; + private _refreshScheduled = false; constructor( private _appOperations: ApplicationOperations, @@ -69,7 +71,6 @@ export class DirectiveExplorerComponent implements OnInit { } ngOnInit(): void { - // setInterval(() => this.refresh(), 1); this.subscribeToBackendEvents(); } @@ -95,7 +96,19 @@ export class DirectiveExplorerComponent implements OnInit { this.forest = view.forest; this.currentSelectedElement = this._clickedElement; if (view.properties && this.currentSelectedElement) { - this._propResolver.setProperties(this.currentSelectedElement, view.properties); + this._propResolver.setProperties( + this.currentSelectedElement, + view.properties, + () => { + this._requestingNestedProperties = true; + }, + () => { + this._requestingNestedProperties = false; + if (this._refreshScheduled) { + this.refresh(); + } + } + ); } }); @@ -106,7 +119,7 @@ export class DirectiveExplorerComponent implements OnInit { this.highlightIDinTreeFromElement = null; }); - // Only one refresh per 50ms. + // Only one refresh per 0.5 seconds. let buffering = false; this._messageBus.on('componentTreeDirty', () => { if (buffering) { @@ -116,13 +129,18 @@ export class DirectiveExplorerComponent implements OnInit { setTimeout(() => { buffering = false; this.refresh(); - }, 50); + }, 500); }); this.refresh(); } refresh(): void { + if (this._requestingNestedProperties) { + this._refreshScheduled = true; + return; + } this._messageBus.emit('getLatestComponentExplorerView', [this._constructViewQuery()]); + this._refreshScheduled = false; } viewSource(): void { @@ -162,7 +180,7 @@ export class DirectiveExplorerComponent implements OnInit { } return { type: PropertyQueryTypes.Specified, - properties: this._propResolver.getExpandedProperties(), + properties: this._propResolver.getExpandedProperties() || {}, }; } diff --git a/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.spec.ts b/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.spec.ts index 9a28e5356fdfb..efdb7a59ab96e 100644 --- a/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.spec.ts +++ b/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/directive-explorer.spec.ts @@ -1,5 +1,5 @@ import { DirectiveExplorerComponent } from './directive-explorer.component'; -import { ComponentExplorerViewQuery } from 'protocol'; +import { ComponentExplorerViewQuery, PropertyQueryTypes } from 'protocol'; import { IndexedNode } from './directive-forest/index-forest'; import SpyObj = jasmine.SpyObj; import { ElementPropertyResolver } from './property-resolver/element-property-resolver'; @@ -28,8 +28,7 @@ describe('DirectiveExplorerComponent', () => { it('subscribe to backend events', () => { comp.subscribeToBackendEvents(); - expect(messageBusMock.on).toHaveBeenCalledTimes(5); - expect(messageBusMock.on).toHaveBeenCalledWith('elementDirectivesProperties', jasmine.any(Function)); + expect(messageBusMock.on).toHaveBeenCalledTimes(4); expect(messageBusMock.on).toHaveBeenCalledWith('latestComponentExplorerView', jasmine.any(Function)); expect(messageBusMock.on).toHaveBeenCalledWith('highlightComponentInTreeFromElement', jasmine.any(Function)); expect(messageBusMock.on).toHaveBeenCalledWith('removeHighlightFromComponentTree', jasmine.any(Function)); @@ -44,8 +43,7 @@ describe('DirectiveExplorerComponent', () => { it('should emit getLatestComponentExplorerView event with null view query', () => { comp.refresh(); - const nullViewQuery: ComponentExplorerViewQuery = { selectedElement: null, expandedProperties: null }; - expect(messageBusMock.emit).toHaveBeenCalledWith('getLatestComponentExplorerView', [nullViewQuery]); + expect(messageBusMock.emit).toHaveBeenCalledWith('getLatestComponentExplorerView', [undefined]); }); it('should emit getLatestComponentExplorerView event on refresh with view query no properties', () => { @@ -60,12 +58,7 @@ describe('DirectiveExplorerComponent', () => { }; comp.refresh(); expect(comp.currentSelectedElement).toBeTruthy(); - const viewQuery: ComponentExplorerViewQuery = { - // tslint:disable-next-line: no-non-null-assertion - selectedElement: comp.currentSelectedElement!.position, - expandedProperties: {}, - }; - expect(messageBusMock.emit).toHaveBeenCalledWith('getLatestComponentExplorerView', [viewQuery]); + expect(messageBusMock.emit).toHaveBeenCalledWith('getLatestComponentExplorerView', [undefined]); }); }); @@ -77,11 +70,19 @@ describe('DirectiveExplorerComponent', () => { }); it('fires node selection events', () => { - nodeMock.position = [0]; + const position = [0]; + nodeMock.position = position; comp.handleNodeSelection(nodeMock); expect(messageBusMock.emit).toHaveBeenCalledTimes(2); - expect(messageBusMock.emit).toHaveBeenCalledWith('getElementDirectivesProperties', [nodeMock.position]); expect(messageBusMock.emit).toHaveBeenCalledWith('setSelectedComponent', [nodeMock.position]); + expect(messageBusMock.emit).toHaveBeenCalledWith('getLatestComponentExplorerView', [ + { + selectedElement: position, + propertyQuery: { + type: PropertyQueryTypes.All, + }, + }, + ]); }); }); }); diff --git a/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/directive-property-resolver.ts b/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/directive-property-resolver.ts index 44307442517f2..293bfda54cc9a 100644 --- a/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/directive-property-resolver.ts +++ b/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/directive-property-resolver.ts @@ -40,13 +40,17 @@ export class DirectivePropertyResolver { this._treeFlattener, this._treeControl, this._directivePosition, - this._messageBus + this._messageBus, + this._onRequestingNestedProperties, + this._onReceivedNestedProperties ); constructor( private _messageBus: MessageBus, private _props: Properties, - private _directivePosition: DirectivePosition + private _directivePosition: DirectivePosition, + private _onRequestingNestedProperties: () => void, + private _onReceivedNestedProperties: () => void ) {} getDirectiveControls(): { dataSource: PropertyDataSource; treeControl: FlatTreeControl } { diff --git a/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/element-property-resolver.spec.ts b/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/element-property-resolver.spec.ts index 94b26d8fa32f4..b199e0c361a70 100644 --- a/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/element-property-resolver.spec.ts +++ b/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/element-property-resolver.spec.ts @@ -69,17 +69,22 @@ describe('ElementPropertyResolver', () => { it('should register directives', () => { const resolver = new ElementPropertyResolver(messageBusMock); - resolver.setProperties(mockIndexedNode, { - FooCmp: { - props: {}, - }, - BarDir: { - props: {}, - }, - BazDir: { - props: {}, + resolver.setProperties( + mockIndexedNode, + { + FooCmp: { + props: {}, + }, + BarDir: { + props: {}, + }, + BazDir: { + props: {}, + }, }, - }); + () => {}, + () => {} + ); expect(resolver.getDirectiveController('FooCmp')).not.toBeFalsy(); expect(resolver.getDirectiveController('BarDir')).not.toBeFalsy(); expect(resolver.getDirectiveController('BazDir')).not.toBeFalsy(); @@ -87,13 +92,18 @@ describe('ElementPropertyResolver', () => { it('should provide nested props', () => { const resolver = new ElementPropertyResolver(messageBusMock); - resolver.setProperties(mockIndexedNode, { - FooCmp: fooNestedProperties, - BarDir: barNestedProps, - BazDir: { - props: {}, + resolver.setProperties( + mockIndexedNode, + { + FooCmp: fooNestedProperties, + BarDir: barNestedProps, + BazDir: { + props: {}, + }, }, - }); + () => {}, + () => {} + ); const fooController = resolver.getDirectiveController('FooCmp'); expect(fooController).toBeTruthy(); // tslint:disable-next-line: no-non-null-assertion diff --git a/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/element-property-resolver.ts b/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/element-property-resolver.ts index 10436bb5a6216..70fd3d49bd311 100644 --- a/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/element-property-resolver.ts +++ b/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/element-property-resolver.ts @@ -32,7 +32,12 @@ export class ElementPropertyResolver { this._directivePropertiesController = new Map(); } - setProperties(indexedNode: IndexedNode, data: DirectivesProperties): void { + setProperties( + indexedNode: IndexedNode, + data: DirectivesProperties, + onRequestProps: () => void, + onReceivedProps: () => void + ): void { // To prevent memory leaks when a directive no longer exists on an element const currentProps = [...this._directivePropertiesController.keys()]; const incomingProps = new Set(Object.keys(data)); @@ -57,7 +62,7 @@ export class ElementPropertyResolver { } this._directivePropertiesController.set( key, - new DirectivePropertyResolver(this._messageBus, data[key], position) + new DirectivePropertyResolver(this._messageBus, data[key], position, onRequestProps, onReceivedProps) ); }); } diff --git a/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/property-data-source.ts b/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/property-data-source.ts index 49d6385b34492..f22a16c9519a9 100644 --- a/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/property-data-source.ts +++ b/projects/ng-devtools/src/lib/devtools-tabs/directive-explorer/property-resolver/property-data-source.ts @@ -9,7 +9,7 @@ import { diff } from '../../diffing'; import { FlatNode, Property } from './element-property-resolver'; const trackBy = (_: number, item: FlatNode) => { - return `#${item.prop.name}#${item.level}#${item.prop.descriptor.value}`; + return `#${item.prop.name}#${item.level}`; }; export class PropertyDataSource extends DataSource { @@ -23,7 +23,9 @@ export class PropertyDataSource extends DataSource { private _treeFlattener: MatTreeFlattener, private _treeControl: FlatTreeControl, private _entityPosition: DirectivePosition, - private _messageBus: MessageBus + private _messageBus: MessageBus, + private _onRequestingNestedProperties: () => void, + private _onReceivedNestedProperties: () => void ) { super(); this._data.next(this._treeFlattener.flattenNodes(this._arrayify(props))); @@ -93,6 +95,7 @@ export class PropertyDataSource extends DataSource { } parentPath = parentPath.reverse(); + this._onRequestingNestedProperties(); this._messageBus.emit('getNestedProperties', [this._entityPosition, parentPath]); this._messageBus.once('nestedProperties', (position: DirectivePosition, data: Properties, path: string[]) => { @@ -103,6 +106,7 @@ export class PropertyDataSource extends DataSource { flatNodes.forEach(f => (f.level += node.level + 1)); this.data.splice(index + 1, 0, ...flatNodes); this._data.next(this.data); + this._onReceivedNestedProperties(); }); }