From 88c409e9969dadcc7cb4c0ece8f94c7c89455761 Mon Sep 17 00:00:00 2001 From: Henning Dieterichs Date: Fri, 21 Apr 2023 10:17:31 +0200 Subject: [PATCH 1/3] Observed deriveds are now recomputed lazily. --- src/vs/base/common/observableImpl/autorun.ts | 87 +- src/vs/base/common/observableImpl/base.ts | 107 ++- src/vs/base/common/observableImpl/derived.ts | 183 ++-- src/vs/base/common/observableImpl/utils.ts | 8 +- src/vs/base/test/common/observable.test.ts | 833 +++++++++++++------ 5 files changed, 842 insertions(+), 376 deletions(-) diff --git a/src/vs/base/common/observableImpl/autorun.ts b/src/vs/base/common/observableImpl/autorun.ts index 6efe47367839f..bff1ad7e4bcf4 100644 --- a/src/vs/base/common/observableImpl/autorun.ts +++ b/src/vs/base/common/observableImpl/autorun.ts @@ -49,8 +49,22 @@ export function autorunWithStore( }); } +const enum AutorunState { + /** + * A dependency could have changed. + * We need to explicitly ask them if at least one dependency changed. + */ + dependenciesMightHaveChanged = 1, + + /** + * A dependency changed and we need to recompute. + */ + stale = 2, + upToDate = 3, +} + export class AutorunObserver implements IObserver, IReader, IDisposable { - public needsToRun = true; + private state = AutorunState.stale; private updateCount = 0; private disposed = false; @@ -76,51 +90,76 @@ export class AutorunObserver implements IObserver, IReader, IDisposable { this.runIfNeeded(); } - public subscribeTo(observable: IObservable) { + public readObservable(observable: IObservable): T { // In case the run action disposes the autorun if (this.disposed) { - return; + return observable.get(); } - this._dependencies.add(observable); - if (!this.staleDependencies.delete(observable)) { - observable.addObserver(this); - } - } - public handleChange(observable: IObservable, change: TChange): void { - const shouldReact = this._handleChange ? this._handleChange({ - changedObservable: observable, - change, - didChange: o => o === observable as any, - }) : true; - this.needsToRun = this.needsToRun || shouldReact; - - if (this.updateCount === 0) { - this.runIfNeeded(); - } + observable.addObserver(this); + const value = observable.get(); + this._dependencies.add(observable); + this.staleDependencies.delete(observable); + return value; } public beginUpdate(): void { + if (this.state === AutorunState.upToDate) { + this.state = AutorunState.dependenciesMightHaveChanged; + } this.updateCount++; } public endUpdate(): void { + if (this.updateCount === 1) { + do { + if (this.state === AutorunState.dependenciesMightHaveChanged) { + this.state = AutorunState.upToDate; + for (const d of this._dependencies) { + d.reportChanges(); + if (this.state as AutorunState === AutorunState.stale) { + // The other dependencies will refresh on demand + break; + } + } + } + + this.runIfNeeded(); + } while (this.state !== AutorunState.upToDate); + } this.updateCount--; - if (this.updateCount === 0) { - this.runIfNeeded(); + } + + public handlePossibleChange(observable: IObservable): void { + if (this.state === AutorunState.upToDate && this.dependencies.has(observable)) { + this.state = AutorunState.dependenciesMightHaveChanged; } } - private runIfNeeded(): void { - if (!this.needsToRun) { + public handleChange(observable: IObservable, change: TChange): void { + if (this.dependencies.has(observable)) { + const shouldReact = this._handleChange ? this._handleChange({ + changedObservable: observable, + change, + didChange: o => o === observable as any, + }) : true; + if (shouldReact) { + this.state = AutorunState.stale; + } + } + } + + private runIfNeeded() { + if (this.state === AutorunState.upToDate) { return; } + // Assert: this.staleDependencies is an empty set. const emptySet = this.staleDependencies; this.staleDependencies = this._dependencies; this._dependencies = emptySet; - this.needsToRun = false; + this.state = AutorunState.upToDate; getLogger()?.handleAutorunTriggered(this); diff --git a/src/vs/base/common/observableImpl/base.ts b/src/vs/base/common/observableImpl/base.ts index 1327a0cf9b997..8003e3d498698 100644 --- a/src/vs/base/common/observableImpl/base.ts +++ b/src/vs/base/common/observableImpl/base.ts @@ -17,9 +17,8 @@ export interface IObservable { */ get(): T; - /** - * Adds an observer. - */ + reportChanges(): void; + addObserver(observer: IObserver): void; removeObserver(observer: IObserver): void; @@ -31,42 +30,51 @@ export interface IObservable { map(fn: (value: T, reader: IReader) => TNew): IObservable; readonly debugName: string; + + /*get isPossiblyStale(): boolean; + + get isUpdating(): boolean;*/ } export interface IReader { /** - * Reports an observable that was read. + * Reads the value of an observable and subscribes to it. * * Is called by {@link IObservable.read}. */ - subscribeTo(observable: IObservable): void; + readObservable(observable: IObservable): T; } export interface IObserver { /** - * Indicates that an update operation is about to begin. - * - * During an update, invariants might not hold for subscribed observables and - * change events might be delayed. - * However, all changes must be reported before all update operations are over. + * Indicates that calling {@link IObservable.get} might return a different value and the observable is in updating mode. + * Must not be called when the given observable has already been reported to be in updating mode. */ beginUpdate(observable: IObservable): void; + /** + * Is called by a subscribed observable when it leaves updating mode and it doesn't expect changes anymore, + * i.e. when a transaction for that observable is over. + * + * Call {@link IObservable.reportChanges} to learn about possible changes (if they weren't reported yet). + */ + endUpdate(observable: IObservable): void; + + handlePossibleChange(observable: IObservable): void; + /** * Is called by a subscribed observable immediately after it notices a change. * - * When {@link IObservable.get} returns and no change has been reported, - * there has been no change for that observable. + * When {@link IObservable.get} is called two times and no change was reported before the second call returns, + * there has been no change in between the two calls for that observable. + * + * If the update counter is zero for a subscribed observable and calling {@link IObservable.get} didn't trigger a change, + * subsequent calls to {@link IObservable.get} don't trigger a change either until the update counter is increased again. * * Implementations must not call into other observables! - * The change should be processed when {@link IObserver.endUpdate} is called. + * The change should be processed when all observed observables settled. */ handleChange(observable: IObservable, change: TChange): void; - - /** - * Indicates that an update operation has completed. - */ - endUpdate(observable: IObservable): void; } export interface ISettable { @@ -97,13 +105,21 @@ export abstract class ConvenientObservable implements IObservable void, getDebugName?: () => } } -export function getFunctionName(fn: Function): string | undefined { - const fnSrc = fn.toString(); - // Pattern: /** @description ... */ - const regexp = /\/\*\*\s*@description\s*([^*]*)\*\//; - const match = regexp.exec(fnSrc); - const result = match ? match[1] : undefined; - return result?.trim(); -} - export class TransactionImpl implements ITransaction { private updatingObservers: { observer: IObserver; observable: IObservable }[] | null = []; @@ -176,10 +183,7 @@ export class TransactionImpl implements ITransaction { return getFunctionName(this.fn); } - public updateObserver( - observer: IObserver, - observable: IObservable - ): void { + public updateObserver(observer: IObserver, observable: IObservable): void { this.updatingObservers!.push({ observer, observable }); observer.beginUpdate(observable); } @@ -194,6 +198,15 @@ export class TransactionImpl implements ITransaction { } } +export function getFunctionName(fn: Function): string | undefined { + const fnSrc = fn.toString(); + // Pattern: /** @description ... */ + const regexp = /\/\*\*\s*@description\s*([^*]*)\*\//; + const match = regexp.exec(fnSrc); + const result = match ? match[1] : undefined; + return result?.trim(); +} + export interface ISettableObservable extends IObservable, ISettable { } @@ -211,7 +224,6 @@ export class ObservableValue super(); this._value = initialValue; } - public get(): T { return this._value; } @@ -221,20 +233,23 @@ export class ObservableValue return; } + let _tx: TransactionImpl | undefined; if (!tx) { - transaction((tx) => { - this.set(value, tx, change); - }, () => `Setting ${this.debugName}`); - return; + tx = _tx = new TransactionImpl(() => { }, () => `Setting ${this.debugName}`); } - - const oldValue = this._value; - this._setValue(value); - getLogger()?.handleObservableChanged(this, { oldValue, newValue: value, change, didChange: true }); - - for (const observer of this.observers) { - tx.updateObserver(observer, this); - observer.handleChange(this, change); + try { + const oldValue = this._value; + this._setValue(value); + getLogger()?.handleObservableChanged(this, { oldValue, newValue: value, change, didChange: true }); + + for (const observer of this.observers) { + tx.updateObserver(observer, this); + observer.handleChange(this, change); + } + } finally { + if (_tx) { + _tx.finish(); + } } } diff --git a/src/vs/base/common/observableImpl/derived.ts b/src/vs/base/common/observableImpl/derived.ts index 84a93132f1544..8c0387a251f02 100644 --- a/src/vs/base/common/observableImpl/derived.ts +++ b/src/vs/base/common/observableImpl/derived.ts @@ -12,9 +12,25 @@ export function derived(debugName: string | (() => string), computeFn: (reade _setDerived(derived); +const enum DerivedState { + /** Initial state, no previous value, recomputation needed */ + initial = 0, + + /** + * A dependency could have changed. + * We need to explicitly ask them if at least one dependency changed. + */ + dependenciesMightHaveChanged = 1, + + /** + * A dependency changed and we need to recompute. + */ + stale = 2, + upToDate = 3, +} + export class Derived extends BaseObservable implements IReader, IObserver { - private hadValue = false; - private hasValue = false; + private state = DerivedState.initial; private value: T | undefined = undefined; private updateCount = 0; @@ -46,8 +62,7 @@ export class Derived extends BaseObservable implements IReader, IObs * We are not tracking changes anymore, thus we have to assume * that our cache is invalid. */ - this.hasValue = false; - this.hadValue = false; + this.state = DerivedState.stale; this.value = undefined; for (const d of this._dependencies) { d.removeObserver(this); @@ -57,108 +72,136 @@ export class Derived extends BaseObservable implements IReader, IObs public get(): T { if (this.observers.size === 0) { - // Cache is not valid and don't refresh the cache. - // Observables should not be read in non-reactive contexts. + // Without observers, we don't know when to clean up stuff. + // Thus, we don't cache anything to prevent memory leaks. const result = this.computeFn(this); // Clear new dependencies this.onLastObserverRemoved(); return result; + } else { + do { + if (this.state === DerivedState.dependenciesMightHaveChanged) { + // We might not get a notification for a dependency that changed while it is updating, + // thus we also have to ask all our depedencies if they changed in this case. + this.state = DerivedState.upToDate; + + for (const d of this._dependencies) { + /** might call {@link handleChange} indirectly, which could invalidate us */ + d.reportChanges(); + + if (this.state as DerivedState === DerivedState.stale) { + // The other dependencies will refresh on demand, so early break + break; + } + } + } + + this._recomputeIfNeeded(); + } while (this.state !== DerivedState.upToDate); + return this.value!; } + } - if (this.updateCount > 0 && this.hasValue) { - // Refresh dependencies - for (const d of this._dependencies) { - // Maybe `.get()` triggers `handleChange`? - d.get(); - if (!this.hasValue) { - // The other dependencies will refresh on demand - break; - } + private _recomputeIfNeeded() { + if (this.state === DerivedState.upToDate) { + return; + } + const emptySet = this.staleDependencies; + this.staleDependencies = this._dependencies; + this._dependencies = emptySet; + + const hadValue = this.state !== DerivedState.initial; + const oldValue = this.value; + this.state = DerivedState.upToDate; + + try { + /** might call {@link handleChange} indirectly, which could invalidate us */ + this.value = this.computeFn(this); + } finally { + // We don't want our observed observables to think that they are (not even temporarily) not being observed. + // Thus, we only unsubscribe from observables that are definitely not read anymore. + for (const o of this.staleDependencies) { + o.removeObserver(this); } + this.staleDependencies.clear(); } - if (!this.hasValue) { - const emptySet = this.staleDependencies; - this.staleDependencies = this._dependencies; - this._dependencies = emptySet; - - const oldValue = this.value; - try { - this.value = this.computeFn(this); - } finally { - // We don't want our observed observables to think that they are (not even temporarily) not being observed. - // Thus, we only unsubscribe from observables that are definitely not read anymore. - for (const o of this.staleDependencies) { - o.removeObserver(this); - } - this.staleDependencies.clear(); - } + const didChange = hadValue && oldValue !== this.value; - this.hasValue = true; - const didChange = this.hadValue && oldValue !== this.value; - getLogger()?.handleDerivedRecomputed(this, { - oldValue, - newValue: this.value, - change: undefined, - didChange - }); - if (didChange) { - for (const r of this.observers) { - r.handleChange(this, undefined); - } + getLogger()?.handleDerivedRecomputed(this, { + oldValue, + newValue: this.value, + change: undefined, + didChange + }); + + if (didChange) { + for (const r of this.observers) { + r.handleChange(this, undefined); } } - return this.value!; } // IObserver Implementation public beginUpdate(): void { + const prevState = this.state; + this.state = DerivedState.dependenciesMightHaveChanged; if (this.updateCount === 0) { for (const r of this.observers) { r.beginUpdate(this); } + } else if (prevState === DerivedState.upToDate) { + for (const r of this.observers) { + r.handlePossibleChange(this); + } } this.updateCount++; } - public handleChange( - _observable: IObservable, - _change: TChange - ): void { - if (this.hasValue) { - this.hadValue = true; - this.hasValue = false; + public endUpdate(): void { + this.updateCount--; + if (this.updateCount === 0) { + for (const r of this.observers) { + r.endUpdate(this); + } } + } - // Not in transaction: Recompute & inform observers immediately - if (this.updateCount === 0 && this.observers.size > 0) { - this.get(); + public handlePossibleChange(observable: IObservable): void { + if (this._dependencies.has(observable)) { + if (this.state === DerivedState.upToDate) { + // In all other states, observers already know that we might have changed. + for (const r of this.observers) { + r.handlePossibleChange(this); + } + } + this.state = DerivedState.dependenciesMightHaveChanged; } - - // Otherwise, recompute in `endUpdate` or on demand. } - public endUpdate(): void { - this.updateCount--; - if (this.updateCount === 0) { - if (this.observers.size > 0) { - // Propagate invalidation - this.get(); - } + public handleChange(observable: IObservable, _change: TChange): void { + if ((this.state === DerivedState.dependenciesMightHaveChanged || this.state === DerivedState.upToDate) && this._dependencies.has(observable)) { + const prevState = this.state; + this.state = DerivedState.stale; - for (const r of this.observers) { - r.endUpdate(this); + if (prevState === DerivedState.upToDate) { + for (const r of this.observers) { + r.handlePossibleChange(this); + } } } } // IReader Implementation - public subscribeTo(observable: IObservable) { + public readObservable(observable: IObservable): T { + // Subscribe before getting the value to enable caching + observable.addObserver(this); + /** This might call {@link handleChange} indirectly, which could invalidate us */ + const value = observable.get(); + // Which is why we only add the observable to the dependencies now. this._dependencies.add(observable); - // We are already added as observer for stale dependencies. - if (!this.staleDependencies.delete(observable)) { - observable.addObserver(this); - } + this.staleDependencies.delete(observable); + return value; } override toString(): string { diff --git a/src/vs/base/common/observableImpl/utils.ts b/src/vs/base/common/observableImpl/utils.ts index de5e6c73f14d9..444e6919ac902 100644 --- a/src/vs/base/common/observableImpl/utils.ts +++ b/src/vs/base/common/observableImpl/utils.ts @@ -296,11 +296,15 @@ class KeepAliveObserver implements IObserver { // NO OP } - handleChange(observable: IObservable, change: TChange): void { + endUpdate(observable: IObservable): void { // NO OP } - endUpdate(observable: IObservable): void { + handlePossibleChange(observable: IObservable): void { + // NO OP + } + + handleChange(observable: IObservable, change: TChange): void { // NO OP } } diff --git a/src/vs/base/test/common/observable.test.ts b/src/vs/base/test/common/observable.test.ts index f1bec25aa50a2..e4aa434878ba3 100644 --- a/src/vs/base/test/common/observable.test.ts +++ b/src/vs/base/test/common/observable.test.ts @@ -5,244 +5,319 @@ import * as assert from 'assert'; import { Emitter } from 'vs/base/common/event'; -import { ISettableObservable, autorun, derived, ITransaction, observableFromEvent, observableValue, transaction } from 'vs/base/common/observable'; +import { ISettableObservable, autorun, derived, ITransaction, observableFromEvent, observableValue, transaction, keepAlive } from 'vs/base/common/observable'; import { BaseObservable, IObservable, IObserver } from 'vs/base/common/observableImpl/base'; -suite('observable integration', () => { - test('basic observable + autorun', () => { - const log = new Log(); - const observable = observableValue('MyObservableValue', 0); - - autorun('MyAutorun', (reader) => { - log.log(`value: ${observable.read(reader)}`); - }); - assert.deepStrictEqual(log.getAndClearEntries(), ['value: 0']); +suite('observables', () => { + /** + * Reads these tests to understand how to use observables. + */ + suite('tutorial', () => { + test('observable + autorun', () => { + const log = new Log(); + const myObservable = observableValue('myObservable', 0); - observable.set(1, undefined); - assert.deepStrictEqual(log.getAndClearEntries(), ['value: 1']); + autorun('myAutorun', (reader) => { + log.log(`myAutorun.run(myObservable: ${myObservable.read(reader)})`); + }); + // The autorun runs immediately + assert.deepStrictEqual(log.getAndClearEntries(), ['myAutorun.run(myObservable: 0)']); - observable.set(1, undefined); - assert.deepStrictEqual(log.getAndClearEntries(), []); + myObservable.set(1, undefined); + // The autorun runs again when any read observable changed + assert.deepStrictEqual(log.getAndClearEntries(), ['myAutorun.run(myObservable: 1)']); - transaction((tx) => { - observable.set(2, tx); + myObservable.set(1, undefined); + // But only if the value changed assert.deepStrictEqual(log.getAndClearEntries(), []); - observable.set(3, tx); - assert.deepStrictEqual(log.getAndClearEntries(), []); + // Transactions batch autorun runs + transaction((tx) => { + myObservable.set(2, tx); + // No auto-run ran yet, even though the value changed + assert.deepStrictEqual(log.getAndClearEntries(), []); + + myObservable.set(3, tx); + assert.deepStrictEqual(log.getAndClearEntries(), []); + }); + // Only at the end of the transaction the autorun re-runs + assert.deepStrictEqual(log.getAndClearEntries(), ['myAutorun.run(myObservable: 3)']); }); - assert.deepStrictEqual(log.getAndClearEntries(), ['value: 3']); - }); + test('computed + autorun', () => { + const log = new Log(); + const observable1 = observableValue('myObservable1', 0); + const observable2 = observableValue('myObservable2', 0); + + const myDerived = derived('myDerived', (reader) => { + const value1 = observable1.read(reader); + const value2 = observable2.read(reader); + const sum = value1 + value2; + log.log(`myDerived.recompute: ${value1} + ${value2} = ${sum}`); + return sum; + }); - test('basic computed + autorun', () => { - const log = new Log(); - const observable1 = observableValue('MyObservableValue1', 0); - const observable2 = observableValue('MyObservableValue2', 0); + autorun('myAutorun', (reader) => { + log.log(`myAutorun(myDerived: ${myDerived.read(reader)})`); + }); + // autorun runs immediately + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myDerived.recompute: 0 + 0 = 0", + "myAutorun(myDerived: 0)", + ]); - const computed = derived('computed', (reader) => { - const value1 = observable1.read(reader); - const value2 = observable2.read(reader); - const sum = value1 + value2; - log.log(`recompute: ${value1} + ${value2} = ${sum}`); - return sum; - }); + observable1.set(1, undefined); + // and on changes... + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myDerived.recompute: 1 + 0 = 1", + "myAutorun(myDerived: 1)", + ]); - autorun('MyAutorun', (reader) => { - log.log(`value: ${computed.read(reader)}`); - }); - assert.deepStrictEqual(log.getAndClearEntries(), [ - 'recompute: 0 + 0 = 0', - 'value: 0', - ]); + observable2.set(1, undefined); + // ... of any dependency. + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myDerived.recompute: 1 + 1 = 2", + "myAutorun(myDerived: 2)", + ]); - observable1.set(1, undefined); - assert.deepStrictEqual(log.getAndClearEntries(), [ - 'recompute: 1 + 0 = 1', - 'value: 1', - ]); + transaction((tx) => { + observable1.set(5, tx); + assert.deepStrictEqual(log.getAndClearEntries(), []); - observable2.set(1, undefined); - assert.deepStrictEqual(log.getAndClearEntries(), [ - 'recompute: 1 + 1 = 2', - 'value: 2', - ]); + observable2.set(5, tx); + assert.deepStrictEqual(log.getAndClearEntries(), []); + }); + // When changing multiple observables in a transaction, + // deriveds are only recomputed on demand. + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myDerived.recompute: 5 + 5 = 10", + "myAutorun(myDerived: 10)", + ]); - transaction((tx) => { - observable1.set(5, tx); - assert.deepStrictEqual(log.getAndClearEntries(), []); + transaction((tx) => { + observable1.set(6, tx); + assert.deepStrictEqual(log.getAndClearEntries(), []); - observable2.set(5, tx); - assert.deepStrictEqual(log.getAndClearEntries(), []); + observable2.set(4, tx); + assert.deepStrictEqual(log.getAndClearEntries(), []); + }); + // Now the autorun didn't run again, because its dependency changed from 10 to 10 (= no change). + assert.deepStrictEqual(log.getAndClearEntries(), (["myDerived.recompute: 6 + 4 = 10"])); }); - assert.deepStrictEqual(log.getAndClearEntries(), [ - 'recompute: 5 + 5 = 10', - 'value: 10', - ]); + test('read during transaction', () => { + const log = new Log(); + const observable1 = observableValue('myObservable1', 0); + const observable2 = observableValue('myObservable2', 0); + + const myDerived = derived('myDerived', (reader) => { + const value1 = observable1.read(reader); + const value2 = observable2.read(reader); + const sum = value1 + value2; + log.log(`myDerived.recompute: ${value1} + ${value2} = ${sum}`); + return sum; + }); - transaction((tx) => { - observable1.set(6, tx); - assert.deepStrictEqual(log.getAndClearEntries(), []); + autorun('myAutorun', (reader) => { + log.log(`myAutorun(myDerived: ${myDerived.read(reader)})`); + }); + // autorun runs immediately + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myDerived.recompute: 0 + 0 = 0", + "myAutorun(myDerived: 0)", + ]); - observable2.set(4, tx); - assert.deepStrictEqual(log.getAndClearEntries(), []); + transaction((tx) => { + observable1.set(-10, tx); + assert.deepStrictEqual(log.getAndClearEntries(), []); + + myDerived.get(); // This forces a (sync) recomputation of the current value + assert.deepStrictEqual(log.getAndClearEntries(), (["myDerived.recompute: -10 + 0 = -10"])); + + observable2.set(10, tx); + assert.deepStrictEqual(log.getAndClearEntries(), []); + }); + // This autorun runs again, because its dependency changed from 0 to -10 and then back to 0. + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myDerived.recompute: -10 + 10 = 0", + "myAutorun(myDerived: 0)", + ]); }); - assert.deepStrictEqual(log.getAndClearEntries(), ['recompute: 6 + 4 = 10']); - }); + test('get without observers', () => { + const log = new Log(); + const observable1 = observableValue('myObservableValue1', 0); + const computed1 = derived('computed', (reader) => { + const value1 = observable1.read(reader); + const result = value1 % 3; + log.log(`recompute1: ${value1} % 3 = ${result}`); + return result; + }); + const computed2 = derived('computed', (reader) => { + const value1 = computed1.read(reader); + const result = value1 * 2; + log.log(`recompute2: ${value1} * 2 = ${result}`); + return result; + }); + const computed3 = derived('computed', (reader) => { + const value1 = computed1.read(reader); + const result = value1 * 3; + log.log(`recompute3: ${value1} * 3 = ${result}`); + return result; + }); + const computedSum = derived('computed', (reader) => { + const value1 = computed2.read(reader); + const value2 = computed3.read(reader); + const result = value1 + value2; + log.log(`recompute4: ${value1} + ${value2} = ${result}`); + return result; + }); + assert.deepStrictEqual(log.getAndClearEntries(), []); - test('read during transaction', () => { - const log = new Log(); - const observable1 = observableValue('MyObservableValue1', 0); - const observable2 = observableValue('MyObservableValue2', 0); + observable1.set(1, undefined); + assert.deepStrictEqual(log.getAndClearEntries(), []); - const computed = derived('computed', (reader) => { - const value1 = observable1.read(reader); - const value2 = observable2.read(reader); - const sum = value1 + value2; - log.log(`recompute: ${value1} + ${value2} = ${sum}`); - return sum; - }); + log.log(`value: ${computedSum.get()}`); + assert.deepStrictEqual(log.getAndClearEntries(), [ + 'recompute1: 1 % 3 = 1', + 'recompute2: 1 * 2 = 2', + 'recompute3: 1 * 3 = 3', + 'recompute4: 2 + 3 = 5', + 'value: 5', + ]); - autorun('MyAutorun', (reader) => { - log.log(`value: ${computed.read(reader)}`); - }); + log.log(`value: ${computedSum.get()}`); + // Because there are no observers, the derived values are not cached, but computed from scratch. + assert.deepStrictEqual(log.getAndClearEntries(), [ + 'recompute1: 1 % 3 = 1', + 'recompute2: 1 * 2 = 2', + 'recompute3: 1 * 3 = 3', + 'recompute4: 2 + 3 = 5', + 'value: 5', + ]); - assert.deepStrictEqual(log.getAndClearEntries(), [ - 'recompute: 0 + 0 = 0', - 'value: 0', - ]); + const disposable = keepAlive(computedSum); // Use keepAlive to keep the cache + log.log(`value: ${computedSum.get()}`); + assert.deepStrictEqual(log.getAndClearEntries(), [ + 'recompute1: 1 % 3 = 1', + 'recompute2: 1 * 2 = 2', + 'recompute3: 1 * 3 = 3', + 'recompute4: 2 + 3 = 5', + 'value: 5', + ]); - log.log(`computed is ${computed.get()}`); - assert.deepStrictEqual(log.getAndClearEntries(), ['computed is 0']); + log.log(`value: ${computedSum.get()}`); + assert.deepStrictEqual(log.getAndClearEntries(), [ + 'value: 5', + ]); - transaction((tx) => { - observable1.set(-1, tx); - log.log(`computed is ${computed.get()}`); + observable1.set(2, undefined); + // The keep alive does not force deriveds to be recomputed + assert.deepStrictEqual(log.getAndClearEntries(), ([])); + + log.log(`value: ${computedSum.get()}`); + // Those deriveds are recomputed on demand assert.deepStrictEqual(log.getAndClearEntries(), [ - 'recompute: -1 + 0 = -1', - 'computed is -1', + "recompute1: 2 % 3 = 2", + "recompute2: 2 * 2 = 4", + "recompute3: 2 * 3 = 6", + "recompute4: 4 + 6 = 10", + "value: 10", ]); + log.log(`value: ${computedSum.get()}`); + // ... and then cached again + assert.deepStrictEqual(log.getAndClearEntries(), (["value: 10"])); - log.log(`computed is ${computed.get()}`); - assert.deepStrictEqual(log.getAndClearEntries(), ['computed is -1']); + disposable.dispose(); // Don't forget to dispose the keepAlive to prevent memory leaks - observable2.set(1, tx); - assert.deepStrictEqual(log.getAndClearEntries(), []); + log.log(`value: ${computedSum.get()}`); + // Which disables the cache again + assert.deepStrictEqual(log.getAndClearEntries(), [ + "recompute1: 2 % 3 = 2", + "recompute2: 2 * 2 = 4", + "recompute3: 2 * 3 = 6", + "recompute4: 4 + 6 = 10", + "value: 10", + ]); + + log.log(`value: ${computedSum.get()}`); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "recompute1: 2 % 3 = 2", + "recompute2: 2 * 2 = 4", + "recompute3: 2 * 3 = 6", + "recompute4: 4 + 6 = 10", + "value: 10", + ]); }); - assert.deepStrictEqual(log.getAndClearEntries(), [ - 'recompute: -1 + 1 = 0', - 'value: 0', - ]); }); test('topological order', () => { const log = new Log(); - const observable1 = observableValue('MyObservableValue1', 0); - const observable2 = observableValue('MyObservableValue2', 0); + const myObservable1 = observableValue('myObservable1', 0); + const myObservable2 = observableValue('myObservable2', 0); - const computed1 = derived('computed1', (reader) => { - const value1 = observable1.read(reader); - const value2 = observable2.read(reader); + const myComputed1 = derived('myComputed1', (reader) => { + const value1 = myObservable1.read(reader); + const value2 = myObservable2.read(reader); const sum = value1 + value2; - log.log(`recompute1: ${value1} + ${value2} = ${sum}`); + log.log(`myComputed1.recompute(myObservable1: ${value1} + myObservable2: ${value2} = ${sum})`); return sum; }); - const computed2 = derived('computed2', (reader) => { - const value1 = computed1.read(reader); - const value2 = observable1.read(reader); - const value3 = observable2.read(reader); + const myComputed2 = derived('myComputed2', (reader) => { + const value1 = myComputed1.read(reader); + const value2 = myObservable1.read(reader); + const value3 = myObservable2.read(reader); const sum = value1 + value2 + value3; - log.log(`recompute2: ${value1} + ${value2} + ${value3} = ${sum}`); + log.log(`myComputed2.recompute(myComputed1: ${value1} + myObservable1: ${value2} + myObservable2: ${value3} = ${sum})`); return sum; }); - const computed3 = derived('computed3', (reader) => { - const value1 = computed2.read(reader); - const value2 = observable1.read(reader); - const value3 = observable2.read(reader); + const myComputed3 = derived('myComputed3', (reader) => { + const value1 = myComputed2.read(reader); + const value2 = myObservable1.read(reader); + const value3 = myObservable2.read(reader); const sum = value1 + value2 + value3; - log.log(`recompute3: ${value1} + ${value2} + ${value3} = ${sum}`); + log.log(`myComputed3.recompute(myComputed2: ${value1} + myObservable1: ${value2} + myObservable2: ${value3} = ${sum})`); return sum; }); - autorun('MyAutorun', (reader) => { - log.log(`value: ${computed3.read(reader)}`); + autorun('myAutorun', (reader) => { + log.log(`myAutorun.run(myComputed3: ${myComputed3.read(reader)})`); }); assert.deepStrictEqual(log.getAndClearEntries(), [ - 'recompute1: 0 + 0 = 0', - 'recompute2: 0 + 0 + 0 = 0', - 'recompute3: 0 + 0 + 0 = 0', - 'value: 0', + "myComputed1.recompute(myObservable1: 0 + myObservable2: 0 = 0)", + "myComputed2.recompute(myComputed1: 0 + myObservable1: 0 + myObservable2: 0 = 0)", + "myComputed3.recompute(myComputed2: 0 + myObservable1: 0 + myObservable2: 0 = 0)", + "myAutorun.run(myComputed3: 0)", ]); - observable1.set(1, undefined); + myObservable1.set(1, undefined); assert.deepStrictEqual(log.getAndClearEntries(), [ - 'recompute1: 1 + 0 = 1', - 'recompute2: 1 + 1 + 0 = 2', - 'recompute3: 2 + 1 + 0 = 3', - 'value: 3', + "myComputed1.recompute(myObservable1: 1 + myObservable2: 0 = 1)", + "myComputed2.recompute(myComputed1: 1 + myObservable1: 1 + myObservable2: 0 = 2)", + "myComputed3.recompute(myComputed2: 2 + myObservable1: 1 + myObservable2: 0 = 3)", + "myAutorun.run(myComputed3: 3)", ]); transaction((tx) => { - observable1.set(2, tx); - log.log(`computed2: ${computed2.get()}`); + myObservable1.set(2, tx); + myComputed2.get(); assert.deepStrictEqual(log.getAndClearEntries(), [ - 'recompute1: 2 + 0 = 2', - 'recompute2: 2 + 2 + 0 = 4', - 'computed2: 4', + "myComputed1.recompute(myObservable1: 2 + myObservable2: 0 = 2)", + "myComputed2.recompute(myComputed1: 2 + myObservable1: 2 + myObservable2: 0 = 4)", ]); - observable1.set(3, tx); - log.log(`computed2: ${computed2.get()}`); + myObservable1.set(3, tx); + myComputed2.get(); assert.deepStrictEqual(log.getAndClearEntries(), [ - 'recompute1: 3 + 0 = 3', - 'recompute2: 3 + 3 + 0 = 6', - 'computed2: 6', + "myComputed1.recompute(myObservable1: 3 + myObservable2: 0 = 3)", + "myComputed2.recompute(myComputed1: 3 + myObservable1: 3 + myObservable2: 0 = 6)", ]); }); assert.deepStrictEqual(log.getAndClearEntries(), [ - 'recompute3: 6 + 3 + 0 = 9', - 'value: 9', - ]); - }); - - test('self-disposing autorun', () => { - const log = new Log(); - - const observable1 = new LoggingObservableValue('MyObservableValue1', 0, log); - const observable2 = new LoggingObservableValue('MyObservableValue2', 0, log); - const observable3 = new LoggingObservableValue('MyObservableValue3', 0, log); - - const d = autorun('autorun', (reader) => { - if (observable1.read(reader) >= 2) { - observable2.read(reader); - d.dispose(); - observable3.read(reader); - } - }); - assert.deepStrictEqual(log.getAndClearEntries(), [ - 'MyObservableValue1.firstObserverAdded', - 'MyObservableValue1.get', - ]); - - observable1.set(1, undefined); - assert.deepStrictEqual(log.getAndClearEntries(), [ - 'MyObservableValue1.set (value 1)', - 'MyObservableValue1.get', - ]); - - observable1.set(2, undefined); - assert.deepStrictEqual(log.getAndClearEntries(), [ - 'MyObservableValue1.set (value 2)', - 'MyObservableValue1.get', - 'MyObservableValue2.firstObserverAdded', - 'MyObservableValue2.get', - 'MyObservableValue1.lastObserverRemoved', - 'MyObservableValue2.lastObserverRemoved', - 'MyObservableValue3.get', + "myComputed3.recompute(myComputed2: 6 + myObservable1: 3 + myObservable2: 0 = 9)", + "myAutorun.run(myComputed3: 9)", ]); }); @@ -388,94 +463,381 @@ suite('observable integration', () => { }); }); - test('get without observers', () => { - // Maybe this scenario should not be supported. + test('reading derived in transaction unsubscribes unnecessary observables', () => { + const log = new Log(); + const shouldReadObservable = observableValue('shouldReadMyObs1', true); + const myObs1 = new LoggingObservableValue('myObs1', 0, log); + const myComputed = derived('myComputed', reader => { + log.log('myComputed.recompute'); + if (shouldReadObservable.read(reader)) { + return myObs1.read(reader); + } + return 1; + }); + autorun('myAutorun', reader => { + const value = myComputed.read(reader); + log.log(`myAutorun: ${value}`); + }); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myComputed.recompute", + "myObs1.firstObserverAdded", + "myObs1.get", + "myAutorun: 0", + ]); + + transaction(tx => { + myObs1.set(1, tx); + assert.deepStrictEqual(log.getAndClearEntries(), (["myObs1.set (value 1)"])); + + shouldReadObservable.set(false, tx); + assert.deepStrictEqual(log.getAndClearEntries(), ([])); + + myComputed.get(); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myComputed.recompute", + "myObs1.lastObserverRemoved", + ]); + }); + assert.deepStrictEqual(log.getAndClearEntries(), (["myAutorun: 1"])); + }); + + test('avoid recomputation of deriveds that are no longer read', () => { const log = new Log(); - const observable1 = observableValue('MyObservableValue1', 0); - const computed1 = derived('computed', (reader) => { - const value1 = observable1.read(reader); - const result = value1 % 3; - log.log(`recompute1: ${value1} % 3 = ${result}`); - return result; + + const myObsShouldRead = new LoggingObservableValue('myObsShouldRead', true, log); + const myObs1 = new LoggingObservableValue('myObs1', 0, log); + + const myComputed1 = derived('myComputed1', reader => { + const myObs1Val = myObs1.read(reader); + const result = myObs1Val % 10; + log.log(`myComputed1(myObs1: ${myObs1Val}): Computed ${result}`); + return myObs1Val; }); - const computed2 = derived('computed', (reader) => { - const value1 = computed1.read(reader); - const result = value1 * 2; - log.log(`recompute2: ${value1} * 2 = ${result}`); - return result; + autorun('myAutorun', reader => { + const shouldRead = myObsShouldRead.read(reader); + if (shouldRead) { + const v = myComputed1.read(reader); + log.log(`myAutorun(shouldRead: true, myComputed1: ${v}): run`); + } else { + log.log(`myAutorun(shouldRead: false): run`); + } }); - const computed3 = derived('computed', (reader) => { - const value1 = computed1.read(reader); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObsShouldRead.firstObserverAdded", + "myObsShouldRead.get", + "myObs1.firstObserverAdded", + "myObs1.get", + "myComputed1(myObs1: 0): Computed 0", + "myAutorun(shouldRead: true, myComputed1: 0): run", + ]); + + transaction(tx => { + myObsShouldRead.set(false, tx); + myObs1.set(1, tx); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObsShouldRead.set (value false)", + "myObs1.set (value 1)", + ]); + }); + // myComputed1 should not be recomputed here, even though its dependency myObs1 changed! + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObsShouldRead.get", + "myAutorun(shouldRead: false): run", + "myObs1.lastObserverRemoved", + ]); + + transaction(tx => { + myObsShouldRead.set(true, tx); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObsShouldRead.set (value true)", + ]); + }); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObsShouldRead.get", + "myObs1.firstObserverAdded", + "myObs1.get", + "myComputed1(myObs1: 1): Computed 1", + "myAutorun(shouldRead: true, myComputed1: 1): run", + ]); + }); + + suite('autorun rerun on neutral change', () => { + test('autorun reruns on neutral observable double change', () => { + const log = new Log(); + const myObservable = observableValue('myObservable', 0); - const result = value1 * 3; - log.log(`recompute3: ${value1} * 3 = ${result}`); - return result; + autorun('myAutorun', (reader) => { + log.log(`myAutorun.run(myObservable: ${myObservable.read(reader)})`); + }); + assert.deepStrictEqual(log.getAndClearEntries(), ['myAutorun.run(myObservable: 0)']); + + + transaction((tx) => { + myObservable.set(2, tx); + assert.deepStrictEqual(log.getAndClearEntries(), []); + + myObservable.set(0, tx); + assert.deepStrictEqual(log.getAndClearEntries(), []); + }); + assert.deepStrictEqual(log.getAndClearEntries(), ['myAutorun.run(myObservable: 0)']); }); - const computedSum = derived('computed', (reader) => { - const value1 = computed2.read(reader); - const value2 = computed3.read(reader); - const result = value1 + value2; - log.log(`recompute4: ${value1} + ${value2} = ${result}`); - return result; + test('autorun does not rerun on indirect neutral observable double change', () => { + const log = new Log(); + const myObservable = observableValue('myObservable', 0); + const myDerived = derived('myDerived', (reader) => { + const val = myObservable.read(reader); + log.log(`myDerived.read(myObservable: ${val})`); + return val; + }); + + autorun('myAutorun', (reader) => { + log.log(`myAutorun.run(myDerived: ${myDerived.read(reader)})`); + }); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myDerived.read(myObservable: 0)", + "myAutorun.run(myDerived: 0)" + ]); + + transaction((tx) => { + myObservable.set(2, tx); + assert.deepStrictEqual(log.getAndClearEntries(), []); + + myObservable.set(0, tx); + assert.deepStrictEqual(log.getAndClearEntries(), []); + }); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myDerived.read(myObservable: 0)" + ]); }); - assert.deepStrictEqual(log.getAndClearEntries(), []); + + test('autorun reruns on indirect neutral observable double change when changes propagate', () => { + const log = new Log(); + const myObservable = observableValue('myObservable', 0); + const myDerived = derived('myDerived', (reader) => { + const val = myObservable.read(reader); + log.log(`myDerived.read(myObservable: ${val})`); + return val; + }); + + autorun('myAutorun', (reader) => { + log.log(`myAutorun.run(myDerived: ${myDerived.read(reader)})`); + }); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myDerived.read(myObservable: 0)", + "myAutorun.run(myDerived: 0)" + ]); + + transaction((tx) => { + myObservable.set(2, tx); + assert.deepStrictEqual(log.getAndClearEntries(), []); + + myDerived.get(); // This marks the auto-run as changed + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myDerived.read(myObservable: 2)" + ]); + + myObservable.set(0, tx); + assert.deepStrictEqual(log.getAndClearEntries(), []); + }); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myDerived.read(myObservable: 0)", + "myAutorun.run(myDerived: 0)" + ]); + }); + }); + + test('self-disposing autorun', () => { + const log = new Log(); + + const observable1 = new LoggingObservableValue('myObservable1', 0, log); + const myObservable2 = new LoggingObservableValue('myObservable2', 0, log); + const myObservable3 = new LoggingObservableValue('myObservable3', 0, log); + + const d = autorun('autorun', (reader) => { + if (observable1.read(reader) >= 2) { + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObservable1.set (value 2)", + "myObservable1.get", + ]); + + myObservable2.read(reader); + // First time this observable is read + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObservable2.firstObserverAdded", + "myObservable2.get", + ]); + + d.dispose(); + // Disposing removes all observers + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObservable1.lastObserverRemoved", + "myObservable2.lastObserverRemoved", + ]); + + myObservable3.read(reader); + // This does not subscribe the observable, because the autorun is disposed + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObservable3.get", + ]); + } + }); + assert.deepStrictEqual(log.getAndClearEntries(), [ + 'myObservable1.firstObserverAdded', + 'myObservable1.get', + ]); observable1.set(1, undefined); - assert.deepStrictEqual(log.getAndClearEntries(), []); + assert.deepStrictEqual(log.getAndClearEntries(), [ + 'myObservable1.set (value 1)', + 'myObservable1.get', + ]); + + observable1.set(2, undefined); + // See asserts in the autorun + assert.deepStrictEqual(log.getAndClearEntries(), ([])); + }); + + test('set dependency in derived', () => { + const log = new Log(); - log.log(`value: ${computedSum.get()}`); + const myObservable = new LoggingObservableValue('myObservable', 0, log); + const myComputed = derived('myComputed', reader => { + let value = myObservable.read(reader); + const origValue = value; + log.log(`myComputed(myObservable: ${origValue}): start computing`); + if (value % 3 !== 0) { + value++; + myObservable.set(value, undefined); + } + log.log(`myComputed(myObservable: ${origValue}): finished computing`); + return value; + }); + + autorun('myAutorun', reader => { + const value = myComputed.read(reader); + log.log(`myAutorun(myComputed: ${value})`); + }); assert.deepStrictEqual(log.getAndClearEntries(), [ - 'recompute1: 1 % 3 = 1', - 'recompute2: 1 * 2 = 2', - 'recompute3: 1 * 3 = 3', - 'recompute4: 2 + 3 = 5', - 'value: 5', + "myObservable.firstObserverAdded", + "myObservable.get", + "myComputed(myObservable: 0): start computing", + "myComputed(myObservable: 0): finished computing", + "myAutorun(myComputed: 0)" ]); - log.log(`value: ${computedSum.get()}`); + myObservable.set(1, undefined); assert.deepStrictEqual(log.getAndClearEntries(), [ - 'recompute1: 1 % 3 = 1', - 'recompute2: 1 * 2 = 2', - 'recompute3: 1 * 3 = 3', - 'recompute4: 2 + 3 = 5', - 'value: 5', + "myObservable.set (value 1)", + "myObservable.get", + "myComputed(myObservable: 1): start computing", + "myObservable.set (value 2)", + "myComputed(myObservable: 1): finished computing", + "myObservable.get", + "myComputed(myObservable: 2): start computing", + "myObservable.set (value 3)", + "myComputed(myObservable: 2): finished computing", + "myObservable.get", + "myComputed(myObservable: 3): start computing", + "myComputed(myObservable: 3): finished computing", + "myAutorun(myComputed: 3)", ]); }); -}); -suite('observable details', () => { - test('1', () => { + test('set dependency in autorun', () => { const log = new Log(); + const myObservable = new LoggingObservableValue('myObservable', 0, log); - const shouldReadObservable = observableValue('shouldReadObservable', true); - const observable = new LoggingObservableValue('observable', 0, log); - const computed = derived('test', reader => { - if (shouldReadObservable.read(reader)) { - return observable.read(reader) * 2; + autorun('myAutorun', reader => { + const value = myObservable.read(reader); + log.log(`myAutorun(myObservable: ${value}): start`); + if (value !== 0 && value < 4) { + myObservable.set(value + 1, undefined); } - return 1; + log.log(`myAutorun(myObservable: ${value}): end`); }); - autorun('test', reader => { - const value = computed.read(reader); - log.log(`autorun: ${value}`); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObservable.firstObserverAdded", + "myObservable.get", + "myAutorun(myObservable: 0): start", + "myAutorun(myObservable: 0): end", + ]); + + myObservable.set(1, undefined); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObservable.set (value 1)", + "myObservable.get", + "myAutorun(myObservable: 1): start", + "myObservable.set (value 2)", + "myAutorun(myObservable: 1): end", + "myObservable.get", + "myAutorun(myObservable: 2): start", + "myObservable.set (value 3)", + "myAutorun(myObservable: 2): end", + "myObservable.get", + "myAutorun(myObservable: 3): start", + "myObservable.set (value 4)", + "myAutorun(myObservable: 3): end", + "myObservable.get", + "myAutorun(myObservable: 4): start", + "myAutorun(myObservable: 4): end", + ]); + }); + + test('get in transaction between sets', () => { + const log = new Log(); + const myObservable = new LoggingObservableValue('myObservable', 0, log); + + const myDerived1 = derived('myDerived1', reader => { + const value = myObservable.read(reader); + log.log(`myDerived1(myObservable: ${value}): start computing`); + return value; }); - assert.deepStrictEqual(log.getAndClearEntries(), (["observable.firstObserverAdded", "observable.get", "autorun: 0"])); + const myDerived2 = derived('myDerived2', reader => { + const value = myDerived1.read(reader); + log.log(`myDerived2(myDerived1: ${value}): start computing`); + return value; + }); + + autorun('myAutorun', reader => { + const value = myDerived2.read(reader); + log.log(`myAutorun(myDerived2: ${value})`); + }); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObservable.firstObserverAdded", + "myObservable.get", + "myDerived1(myObservable: 0): start computing", + "myDerived2(myDerived1: 0): start computing", + "myAutorun(myDerived2: 0)", + ]); transaction(tx => { - observable.set(1, tx); - assert.deepStrictEqual(log.getAndClearEntries(), (["observable.set (value 1)"])); + myObservable.set(1, tx); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObservable.set (value 1)", + ]); - shouldReadObservable.set(false, tx); - assert.deepStrictEqual(log.getAndClearEntries(), ([])); + myDerived2.get(); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObservable.get", + "myDerived1(myObservable: 1): start computing", + "myDerived2(myDerived1: 1): start computing", + ]); - computed.get(); - assert.deepStrictEqual(log.getAndClearEntries(), (["observable.lastObserverRemoved"])); + myObservable.set(2, tx); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObservable.set (value 2)", + ]); }); - assert.deepStrictEqual(log.getAndClearEntries(), (["autorun: 1"])); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObservable.get", + "myDerived1(myObservable: 2): start computing", + "myDerived2(myDerived1: 2): start computing", + "myAutorun(myDerived2: 2)", + ]); }); }); @@ -489,13 +851,16 @@ export class LoggingObserver implements IObserver { this.count++; this.log.log(`${this.debugName}.beginUpdate (count ${this.count})`); } - handleChange(observable: IObservable, change: TChange): void { - this.log.log(`${this.debugName}.handleChange (count ${this.count})`); - } endUpdate(observable: IObservable): void { this.log.log(`${this.debugName}.endUpdate (count ${this.count})`); this.count--; } + handleChange(observable: IObservable, change: TChange): void { + this.log.log(`${this.debugName}.handleChange (count ${this.count})`); + } + handlePossibleChange(observable: IObservable): void { + this.log.log(`${this.debugName}.handlePossibleChange`); + } } export class LoggingObservableValue From 6b9698d2b6c3f4426f3083303ec015f1c2975524 Mon Sep 17 00:00:00 2001 From: Henning Dieterichs Date: Fri, 21 Apr 2023 10:35:38 +0200 Subject: [PATCH 2/3] Fixes CI --- src/vs/base/common/event.ts | 61 +++++++++++++++++-- .../audioCues/browser/audioCueService.ts | 36 +---------- 2 files changed, 57 insertions(+), 40 deletions(-) diff --git a/src/vs/base/common/event.ts b/src/vs/base/common/event.ts index 172eeedf22ac7..81981c5290695 100644 --- a/src/vs/base/common/event.ts +++ b/src/vs/base/common/event.ts @@ -599,13 +599,13 @@ export namespace Event { private _counter = 0; private _hasChanged = false; - constructor(readonly obs: IObservable, store: DisposableStore | undefined) { + constructor(readonly _observable: IObservable, store: DisposableStore | undefined) { const options: EmitterOptions = { onWillAddFirstListener: () => { - obs.addObserver(this); + _observable.addObserver(this); }, onDidRemoveLastListener: () => { - obs.removeObserver(this); + _observable.removeObserver(this); } }; if (!store) { @@ -618,28 +618,77 @@ export namespace Event { } beginUpdate(_observable: IObservable): void { - // console.assert(_observable === this.obs); + // assert(_observable === this.obs); this._counter++; } + handlePossibleChange(_observable: IObservable): void { + // assert(_observable === this.obs); + } + handleChange(_observable: IObservable, _change: TChange): void { + // assert(_observable === this.obs); this._hasChanged = true; } endUpdate(_observable: IObservable): void { - if (--this._counter === 0) { + // assert(_observable === this.obs); + this._counter--; + if (this._counter === 0) { + this._observable.reportChanges(); if (this._hasChanged) { this._hasChanged = false; - this.emitter.fire(this.obs.get()); + this.emitter.fire(this._observable.get()); } } } } + /** + * Creates an event emitter that is fired when the observable changes. + * Each listeners subscribes to the emitter. + */ export function fromObservable(obs: IObservable, store?: DisposableStore): Event { const observer = new EmitterObserver(obs, store); return observer.emitter.event; } + + /** + * Each listener is attached to the observable directly. + */ + export function fromObservableLight(observable: IObservable): Event { + return (listener) => { + let count = 0; + let didChange = false; + const observer: IObserver = { + beginUpdate() { + count++; + }, + endUpdate() { + count--; + if (count === 0) { + observable.reportChanges(); + if (didChange) { + didChange = false; + listener(); + } + } + }, + handlePossibleChange() { + // noop + }, + handleChange() { + didChange = true; + } + }; + observable.addObserver(observer); + return { + dispose() { + observable.removeObserver(observer); + } + }; + }; + } } export interface EmitterOptions { diff --git a/src/vs/platform/audioCues/browser/audioCueService.ts b/src/vs/platform/audioCues/browser/audioCueService.ts index 4b1a957dd518b..50ee35c237c24 100644 --- a/src/vs/platform/audioCues/browser/audioCueService.ts +++ b/src/vs/platform/audioCues/browser/audioCueService.ts @@ -10,7 +10,7 @@ import { IConfigurationService } from 'vs/platform/configuration/common/configur import { createDecorator } from 'vs/platform/instantiation/common/instantiation'; import { Event } from 'vs/base/common/event'; import { localize } from 'vs/nls'; -import { IObservable, observableFromEvent, derived, IObserver } from 'vs/base/common/observable'; +import { observableFromEvent, derived } from 'vs/base/common/observable'; export const IAudioCueService = createDecorator('audioCue'); @@ -134,7 +134,7 @@ export class AudioCueService extends Disposable implements IAudioCueService { } public onEnabledChanged(cue: AudioCue): Event { - return eventFromObservable(this.isEnabledCache.get(cue)); + return Event.fromObservableLight(this.isEnabledCache.get(cue)); } } @@ -160,38 +160,6 @@ function playAudio(url: string, volume: number): Promise { }); } -function eventFromObservable(observable: IObservable): Event { - return (listener) => { - let count = 0; - let didChange = false; - const observer: IObserver = { - beginUpdate() { - count++; - }, - endUpdate() { - count--; - if (count === 0 && didChange) { - didChange = false; - listener(); - } - }, - handleChange() { - if (count === 0) { - listener(); - } else { - didChange = true; - } - } - }; - observable.addObserver(observer); - return { - dispose() { - observable.removeObserver(observer); - } - }; - }; -} - class Cache { private readonly map = new Map(); constructor(private readonly getValue: (value: TArg) => TValue) { From b255865f5766e7baef2ae6b8770e0b0be03fcf79 Mon Sep 17 00:00:00 2001 From: Henning Dieterichs Date: Fri, 21 Apr 2023 12:55:42 +0200 Subject: [PATCH 3/3] Fixes observable bug --- src/vs/base/common/observableImpl/derived.ts | 14 +++--- src/vs/base/test/common/observable.test.ts | 52 ++++++++++++++++++++ 2 files changed, 59 insertions(+), 7 deletions(-) diff --git a/src/vs/base/common/observableImpl/derived.ts b/src/vs/base/common/observableImpl/derived.ts index 8c0387a251f02..1145d9e2d485d 100644 --- a/src/vs/base/common/observableImpl/derived.ts +++ b/src/vs/base/common/observableImpl/derived.ts @@ -145,7 +145,9 @@ export class Derived extends BaseObservable implements IReader, IObs // IObserver Implementation public beginUpdate(): void { const prevState = this.state; - this.state = DerivedState.dependenciesMightHaveChanged; + if (this.state === DerivedState.upToDate) { + this.state = DerivedState.dependenciesMightHaveChanged; + } if (this.updateCount === 0) { for (const r of this.observers) { r.beginUpdate(this); @@ -168,12 +170,10 @@ export class Derived extends BaseObservable implements IReader, IObs } public handlePossibleChange(observable: IObservable): void { - if (this._dependencies.has(observable)) { - if (this.state === DerivedState.upToDate) { - // In all other states, observers already know that we might have changed. - for (const r of this.observers) { - r.handlePossibleChange(this); - } + if (this.state === DerivedState.upToDate && this._dependencies.has(observable)) { + // In all other states, observers already know that we might have changed. + for (const r of this.observers) { + r.handlePossibleChange(this); } this.state = DerivedState.dependenciesMightHaveChanged; } diff --git a/src/vs/base/test/common/observable.test.ts b/src/vs/base/test/common/observable.test.ts index e4aa434878ba3..44ddacf5b4554 100644 --- a/src/vs/base/test/common/observable.test.ts +++ b/src/vs/base/test/common/observable.test.ts @@ -839,6 +839,58 @@ suite('observables', () => { "myAutorun(myDerived2: 2)", ]); }); + + test('bug: Dont reset states', () => { + const log = new Log(); + const myObservable1 = new LoggingObservableValue('myObservable1', 0, log); + + const myObservable2 = new LoggingObservableValue('myObservable2', 0, log); + const myDerived2 = derived('myDerived2', reader => { + const val = myObservable2.read(reader); + log.log(`myDerived2.computed(myObservable2: ${val})`); + return val % 10; + }); + + const myDerived3 = derived('myDerived3', reader => { + const val1 = myObservable1.read(reader); + const val2 = myDerived2.read(reader); + log.log(`myDerived3.computed(myDerived1: ${val1}, myDerived2: ${val2})`); + return `${val1} + ${val2}`; + }); + + autorun('myAutorun', reader => { + const val = myDerived3.read(reader); + log.log(`myAutorun(myDerived3: ${val})`); + }); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObservable1.firstObserverAdded", + "myObservable1.get", + "myObservable2.firstObserverAdded", + "myObservable2.get", + "myDerived2.computed(myObservable2: 0)", + "myDerived3.computed(myDerived1: 0, myDerived2: 0)", + "myAutorun(myDerived3: 0 + 0)", + ]); + + transaction(tx => { + myObservable1.set(1, tx); // Mark myDerived 3 as stale + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObservable1.set (value 1)", + ]); + + myObservable2.set(10, tx); // This is a non-change. myDerived3 should not be marked as possibly-depedency-changed! + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObservable2.set (value 10)", + ]); + }); + assert.deepStrictEqual(log.getAndClearEntries(), [ + "myObservable1.get", + "myObservable2.get", + "myDerived2.computed(myObservable2: 10)", + 'myDerived3.computed(myDerived1: 1, myDerived2: 0)', + 'myAutorun(myDerived3: 1 + 0)', + ]); + }); }); export class LoggingObserver implements IObserver {