diff --git a/src/DependencyGraph.spec.ts b/src/DependencyGraph.spec.ts index fd3c5b260..c51dabfee 100644 --- a/src/DependencyGraph.spec.ts +++ b/src/DependencyGraph.spec.ts @@ -142,10 +142,15 @@ describe('DependencyGraph', () => { expect(mock.callCount).to.equal(1); }); - it('emits immediately when configured to do so', () => { - let mock = sinon.mock(); - graph.onchange('a', mock, true); - expect(mock.callCount).to.equal(1); + it('does not cause infinite loop on circular dependency', () => { + //direct + graph.addOrReplace('a', ['b']); + graph.addOrReplace('b', ['a']); + + //indirect + graph.addOrReplace('c', ['d']); + graph.addOrReplace('d', ['e']); + graph.addOrReplace('e', ['c']); }); }); diff --git a/src/DependencyGraph.ts b/src/DependencyGraph.ts index ebb6bae90..889414a67 100644 --- a/src/DependencyGraph.ts +++ b/src/DependencyGraph.ts @@ -9,7 +9,10 @@ export class DependencyGraph { */ public nodes = {} as Record; - private onchangeEmitter = new EventEmitter(); + /** + * An internal event emitter for when keys have changed. + */ + private onchangeEmitter = new EventEmitter(); /** * Add a node to the graph. @@ -26,7 +29,7 @@ export class DependencyGraph { //create a new dependency node let node = new Node(key, dependencies, this); this.nodes[key] = node; - this.onchangeEmitter.emit(key, key); + this.emit(key, { sourceKey: key, notifiedKeys: new Set() }); } /** @@ -70,25 +73,26 @@ export class DependencyGraph { */ public remove(key: string) { delete this.nodes[key]; - this.onchangeEmitter.emit(key, key); + this.emit(key, { sourceKey: key, notifiedKeys: new Set() }); } /** * Emit event that this item has changed */ - public emit(key: string) { - this.onchangeEmitter.emit(key, key); + public emit(key: string, event: DependencyChangedEvent) { + //prevent infinite event loops by skipping already-notified keys + if (!event.notifiedKeys.has(key)) { + event.notifiedKeys.add(key); + this.onchangeEmitter.emit(key, event); + } } /** * Listen for any changes to dependencies with the given key. * @param emitImmediately if true, the handler will be called once immediately. */ - public onchange(key: string, handler: (key) => void, emitImmediately = false) { + public onchange(key: string, handler: (event: DependencyChangedEvent) => void) { this.onchangeEmitter.on(key, handler); - if (emitImmediately) { - this.onchangeEmitter.emit(key, key); - } return () => { this.onchangeEmitter.off(key, handler); }; @@ -103,6 +107,17 @@ export class DependencyGraph { } } +export interface DependencyChangedEvent { + /** + * The key that was the initiator of this event. Child keys will emit this same event object, but this key will remain the same + */ + sourceKey: string; + /** + * A set of keys that have already been notified of this change. Used to prevent circular reference notification cycles + */ + notifiedKeys: Set; +} + export class Node { public constructor( public key: string, @@ -113,9 +128,9 @@ export class Node { this.subscriptions = []; } for (let dependency of this.dependencies) { - let sub = this.graph.onchange(dependency, () => { + let sub = this.graph.onchange(dependency, (event) => { //notify the graph that we changed since one of our dependencies changed - this.graph.emit(this.key); + this.graph.emit(this.key, event); }); this.subscriptions.push(sub); diff --git a/src/Scope.ts b/src/Scope.ts index c3a9b2787..35466901f 100644 --- a/src/Scope.ts +++ b/src/Scope.ts @@ -16,6 +16,7 @@ import { URI } from 'vscode-uri'; import { LogLevel } from './Logger'; import { isBrsFile, isClassStatement, isFunctionStatement, isFunctionType, isXmlFile, isCustomType, isClassMethodStatement } from './astUtils/reflection'; import type { BrsFile } from './files/BrsFile'; +import type { DependencyChangedEvent } from './DependencyGraph'; /** * A class to keep track of all declarations within a given scope (like source scope, component scope) @@ -32,8 +33,10 @@ export class Scope { //anytime a dependency for this scope changes, we need to be revalidated this.programHandles.push( - this.program.dependencyGraph.onchange(this.dependencyGraphKey, this.onDependenciesChanged.bind(this), true) + this.program.dependencyGraph.onchange(this.dependencyGraphKey, this.onDependenciesChanged.bind(this)) ); + //invalidate immediately since this is a new scope + this.invalidate(); } /** @@ -119,8 +122,8 @@ export class Scope { */ protected diagnostics = [] as BsDiagnostic[]; - protected onDependenciesChanged(key: string) { - this.logDebug('invalidated because dependency graph said [', key, '] changed'); + protected onDependenciesChanged(event: DependencyChangedEvent) { + this.logDebug('invalidated because dependency graph said [', event.sourceKey, '] changed'); this.invalidate(); } diff --git a/src/files/XmlFile.ts b/src/files/XmlFile.ts index ddb681d35..c74d93843 100644 --- a/src/files/XmlFile.ts +++ b/src/files/XmlFile.ts @@ -489,7 +489,7 @@ export class XmlFile { if (this.needsTranspiled || extraImportScripts.length > 0) { //temporarily add the missing imports as script tags - const originalScripts = this.ast.component.scripts; + const originalScripts = this.ast.component?.scripts ?? []; this.ast.component.scripts = [ ...originalScripts, ...extraImportScripts