bitwarden/browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser synced 2025-12-15 07:43:35 +00:00
Code Issues Releases Wiki Activity

Remove memory storage cache from derived state. Use observable cache and port messaging (#8939)

Browse Source
This commit is contained in:
Matt Gibson
2024-04-26 15:08:39 -04:00
committed by GitHub
parent b3242145f9
commit 089f251a0c
19 changed files with 83 additions and 294 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
libs/common/src/platform/state/derive-definition.ts
View File

@@ -171,8 +171,8 @@ export class DeriveDefinition<TFrom, TTo, TDeps extends DerivedStateDependencies
return this.options.clearOnCleanup ?? true;
}
buildCacheKey(location: string): string {
return `derived_${location}_${this.stateDefinition.name}_${this.uniqueDerivationName}`;
buildCacheKey(): string {
return `derived_${this.stateDefinition.name}_${this.uniqueDerivationName}`;
}
/**

26
libs/common/src/platform/state/implementations/default-derived-state.provider.ts
View File

@@ -1,11 +1,6 @@
import { Observable } from "rxjs";
import { DerivedStateDependencies } from "../../../types/state";
import {
AbstractStorageService,
ObservableStorageService,
} from "../../abstractions/storage.service";
import { StorageServiceProvider } from "../../services/storage-service.provider";
import { DeriveDefinition } from "../derive-definition";
import { DerivedState } from "../derived-state";
import { DerivedStateProvider } from "../derived-state.provider";
@@ -15,18 +10,14 @@ import { DefaultDerivedState } from "./default-derived-state";
export class DefaultDerivedStateProvider implements DerivedStateProvider {
private cache: Record<string, DerivedState<unknown>> = {};
constructor(protected storageServiceProvider: StorageServiceProvider) {}
constructor() {}
get<TFrom, TTo, TDeps extends DerivedStateDependencies>(
parentState$: Observable<TFrom>,
deriveDefinition: DeriveDefinition<TFrom, TTo, TDeps>,
dependencies: TDeps,
): DerivedState<TTo> {
// TODO: we probably want to support optional normal memory storage for browser
const [location, storageService] = this.storageServiceProvider.get("memory", {
browser: "memory-large-object",
});
const cacheKey = deriveDefinition.buildCacheKey(location);
const cacheKey = deriveDefinition.buildCacheKey();
const existingDerivedState = this.cache[cacheKey];
if (existingDerivedState != null) {
// I have to cast out of the unknown generic but this should be safe if rules
@@ -34,10 +25,7 @@ export class DefaultDerivedStateProvider implements DerivedStateProvider {
return existingDerivedState as DefaultDerivedState<TFrom, TTo, TDeps>;
}
const newDerivedState = this.buildDerivedState(parentState$, deriveDefinition, dependencies, [
location,
storageService,
]);
const newDerivedState = this.buildDerivedState(parentState$, deriveDefinition, dependencies);
this.cache[cacheKey] = newDerivedState;
return newDerivedState;
}
@@ -46,13 +34,7 @@ export class DefaultDerivedStateProvider implements DerivedStateProvider {
parentState$: Observable<TFrom>,
deriveDefinition: DeriveDefinition<TFrom, TTo, TDeps>,
dependencies: TDeps,
storageLocation: [string, AbstractStorageService & ObservableStorageService],
): DerivedState<TTo> {
return new DefaultDerivedState<TFrom, TTo, TDeps>(
parentState$,
deriveDefinition,
storageLocation[1],
dependencies,
);
return new DefaultDerivedState<TFrom, TTo, TDeps>(parentState$, deriveDefinition, dependencies);
}
}

117
libs/common/src/platform/state/implementations/default-derived-state.spec.ts
View File

@@ -5,7 +5,6 @@
import { Subject, firstValueFrom } from "rxjs";
import { awaitAsync, trackEmissions } from "../../../../spec";
import { FakeStorageService } from "../../../../spec/fake-storage.service";
import { DeriveDefinition } from "../derive-definition";
import { StateDefinition } from "../state-definition";
@@ -29,7 +28,6 @@ const deriveDefinition = new DeriveDefinition<string, Date, { date: Date }>(
describe("DefaultDerivedState", () => {
let parentState$: Subject<string>;
let memoryStorage: FakeStorageService;
let sut: DefaultDerivedState<string, Date, { date: Date }>;
const deps = {
date: new Date(),
@@ -38,8 +36,7 @@ describe("DefaultDerivedState", () => {
beforeEach(() => {
callCount = 0;
parentState$ = new Subject();
memoryStorage = new FakeStorageService();
sut = new DefaultDerivedState(parentState$, deriveDefinition, memoryStorage, deps);
sut = new DefaultDerivedState(parentState$, deriveDefinition, deps);
});
afterEach(() => {
@@ -66,71 +63,33 @@ describe("DefaultDerivedState", () => {
expect(callCount).toBe(1);
});
it("should store the derived state in memory", async () => {
const dateString = "2020-01-01";
trackEmissions(sut.state$);
parentState$.next(dateString);
await awaitAsync();
expect(memoryStorage.internalStore[deriveDefinition.storageKey]).toEqual(
derivedValue(new Date(dateString)),
);
const calls = memoryStorage.mock.save.mock.calls;
expect(calls.length).toBe(1);
expect(calls[0][0]).toBe(deriveDefinition.storageKey);
expect(calls[0][1]).toEqual(derivedValue(new Date(dateString)));
});
describe("forceValue", () => {
const initialParentValue = "2020-01-01";
const forced = new Date("2020-02-02");
let emissions: Date[];
describe("without observers", () => {
beforeEach(async () => {
parentState$.next(initialParentValue);
await awaitAsync();
});
it("should store the forced value", async () => {
await sut.forceValue(forced);
expect(memoryStorage.internalStore[deriveDefinition.storageKey]).toEqual(
derivedValue(forced),
);
});
beforeEach(async () => {
emissions = trackEmissions(sut.state$);
parentState$.next(initialParentValue);
await awaitAsync();
});
describe("with observers", () => {
beforeEach(async () => {
emissions = trackEmissions(sut.state$);
parentState$.next(initialParentValue);
await awaitAsync();
});
it("should force the value", async () => {
await sut.forceValue(forced);
expect(emissions).toEqual([new Date(initialParentValue), forced]);
});
it("should store the forced value", async () => {
await sut.forceValue(forced);
expect(memoryStorage.internalStore[deriveDefinition.storageKey]).toEqual(
derivedValue(forced),
);
});
it("should only force the value once", async () => {
await sut.forceValue(forced);
it("should force the value", async () => {
await sut.forceValue(forced);
expect(emissions).toEqual([new Date(initialParentValue), forced]);
});
parentState$.next(initialParentValue);
await awaitAsync();
it("should only force the value once", async () => {
await sut.forceValue(forced);
parentState$.next(initialParentValue);
await awaitAsync();
expect(emissions).toEqual([
new Date(initialParentValue),
forced,
new Date(initialParentValue),
]);
});
expect(emissions).toEqual([
new Date(initialParentValue),
forced,
new Date(initialParentValue),
]);
});
});
@@ -148,42 +107,6 @@ describe("DefaultDerivedState", () => {
expect(parentState$.observed).toBe(false);
});
it("should clear state after cleanup", async () => {
const subscription = sut.state$.subscribe();
parentState$.next(newDate);
await awaitAsync();
expect(memoryStorage.internalStore[deriveDefinition.storageKey]).toEqual(
derivedValue(new Date(newDate)),
);
subscription.unsubscribe();
// Wait for cleanup
await awaitAsync(cleanupDelayMs * 2);
expect(memoryStorage.internalStore[deriveDefinition.storageKey]).toBeUndefined();
});
it("should not clear state after cleanup if clearOnCleanup is false", async () => {
deriveDefinition.options.clearOnCleanup = false;
const subscription = sut.state$.subscribe();
parentState$.next(newDate);
await awaitAsync();
expect(memoryStorage.internalStore[deriveDefinition.storageKey]).toEqual(
derivedValue(new Date(newDate)),
);
subscription.unsubscribe();
// Wait for cleanup
await awaitAsync(cleanupDelayMs * 2);
expect(memoryStorage.internalStore[deriveDefinition.storageKey]).toEqual(
derivedValue(new Date(newDate)),
);
});
it("should not cleanup if there are still subscribers", async () => {
const subscription1 = sut.state$.subscribe();
const sub2Emissions: Date[] = [];
@@ -260,7 +183,3 @@ describe("DefaultDerivedState", () => {
});
});
});
function derivedValue<T>(value: T) {
return { derived: true, value };
}

21
libs/common/src/platform/state/implementations/default-derived-state.ts
View File

@@ -1,10 +1,6 @@
import { Observable, ReplaySubject, Subject, concatMap, merge, share, timer } from "rxjs";
import { DerivedStateDependencies } from "../../../types/state";
import {
AbstractStorageService,
ObservableStorageService,
} from "../../abstractions/storage.service";
import { DeriveDefinition } from "../derive-definition";
import { DerivedState } from "../derived-state";
@@ -22,7 +18,6 @@ export class DefaultDerivedState<TFrom, TTo, TDeps extends DerivedStateDependenc
constructor(
private parentState$: Observable<TFrom>,
protected deriveDefinition: DeriveDefinition<TFrom, TTo, TDeps>,
private memoryStorage: AbstractStorageService & ObservableStorageService,
private dependencies: TDeps,
) {
this.storageKey = deriveDefinition.storageKey;
@@ -34,7 +29,6 @@ export class DefaultDerivedState<TFrom, TTo, TDeps extends DerivedStateDependenc
derivedStateOrPromise = await derivedStateOrPromise;
}
const derivedState = derivedStateOrPromise;
await this.storeValue(derivedState);
return derivedState;
}),
);
@@ -44,26 +38,13 @@ export class DefaultDerivedState<TFrom, TTo, TDeps extends DerivedStateDependenc
connector: () => {
return new ReplaySubject<TTo>(1);
},
resetOnRefCountZero: () =>
timer(this.deriveDefinition.cleanupDelayMs).pipe(
concatMap(async () => {
if (this.deriveDefinition.clearOnCleanup) {
await this.memoryStorage.remove(this.storageKey);
}
return true;
}),
),
resetOnRefCountZero: () => timer(this.deriveDefinition.cleanupDelayMs),
}),
);
}
async forceValue(value: TTo) {
await this.storeValue(value);
this.forcedValueSubject.next(value);
return value;
}
private storeValue(value: TTo) {
return this.memoryStorage.save(this.storageKey, { derived: true, value });
}
}