From 06918bb4675ffbff7c7d040ee0271390388c2776 Mon Sep 17 00:00:00 2001 From: Victor Oliva Date: Mon, 10 Jul 2023 07:36:07 +0200 Subject: [PATCH] add subinstance tests (#301) --- .../context-state2/src/internal/state-node.ts | 8 +- .../context-state2/src/subinstance.test.ts | 304 +++++++++++++++--- packages/context-state2/src/subinstance.ts | 77 +++-- packages/context-state2/src/substate.ts | 7 + 4 files changed, 323 insertions(+), 73 deletions(-) diff --git a/packages/context-state2/src/internal/state-node.ts b/packages/context-state2/src/internal/state-node.ts index 7a6ea24..700ed26 100644 --- a/packages/context-state2/src/internal/state-node.ts +++ b/packages/context-state2/src/internal/state-node.ts @@ -108,6 +108,11 @@ export function createStateNode( key: K }>() const addInstance = (key: K) => { + const orderedKey = nestedMapKey(key) + if (instances.get(orderedKey)) { + return + } + // Wait until parents have emitted a value const parent$ = defer(() => { const instances = parents.map((parent) => parent.getInstance(key)) @@ -127,9 +132,8 @@ export function createStateNode( return of(null) }) - // TODO case key already has instance? instances.set( - nestedMapKey(key), + orderedKey, createInstance( key, parent$.pipe( diff --git a/packages/context-state2/src/subinstance.test.ts b/packages/context-state2/src/subinstance.test.ts index 3862773..762efee 100644 --- a/packages/context-state2/src/subinstance.test.ts +++ b/packages/context-state2/src/subinstance.test.ts @@ -1,55 +1,271 @@ -import { Subject, filter, map, startWith } from "rxjs" -import { InstanceUpdate, createRoot, subinstance } from "./" +import { + NEVER, + Subject, + filter, + from, + map, + mergeAll, + of, + startWith, +} from "rxjs" +import { InstanceUpdate, createRoot, subinstance, substate } from "./" describe("subinstance", () => { - it("works", () => { - const root = createRoot() - const instance$ = new Subject>() - const updates$ = new Subject<{ key: string; value: string }>() - const [instanceNode] = subinstance( - root, - "keyName", - () => instance$, - (id) => - updates$.pipe( - filter((v) => v.key === id), - map((v) => v.value), - startWith(id), + describe("behaviour", () => { + it("creates instances with the specified key name", () => { + const root = createRoot() + const instance$ = new Subject>() + const updates$ = new Subject<{ key: string; value: string }>() + const [instanceNode] = subinstance( + root, + "keyName", + () => instance$, + (id) => + updates$.pipe( + filter((v) => v.key === id), + map((v) => v.value), + startWith(id), + ), + ) + root.run() + + instance$.next({ + type: "add", + key: "a", + }) + expect(instanceNode.getValue({ keyName: "a" })).toEqual("a") + + instance$.next({ + type: "add", + key: "b", + }) + expect(instanceNode.getValue({ keyName: "a" })).toEqual("a") + expect(instanceNode.getValue({ keyName: "b" })).toEqual("b") + + updates$.next({ + key: "a", + value: "new A value", + }) + expect(instanceNode.getValue({ keyName: "a" })).toEqual("new A value") + expect(instanceNode.getValue({ keyName: "b" })).toEqual("b") + + instance$.next({ + type: "remove", + key: "a", + }) + expect(() => instanceNode.getValue({ keyName: "a" })).toThrow() + expect(instanceNode.getValue({ keyName: "b" })).toEqual("b") + + instance$.next({ + type: "remove", + key: "b", + }) + expect(() => instanceNode.getValue({ keyName: "b" })).toThrow() + }) + + it("throws if the key name is already used by one of the parents", () => { + const root = createRoot("rootKey") + const context = substate(root, () => NEVER) + + expect(() => + subinstance( + root, + "rootKey", + () => NEVER, + () => NEVER, ), - ) - root.run() - - instance$.next({ - type: "add", - key: "a", + ).toThrow() + expect(() => + subinstance( + context, + "rootKey", + () => NEVER, + () => NEVER, + ), + ).toThrow() }) - expect(instanceNode.getValue({ keyName: "a" })).toEqual("a") - instance$.next({ - type: "add", - key: "b", - }) - expect(instanceNode.getValue({ keyName: "a" })).toEqual("a") - expect(instanceNode.getValue({ keyName: "b" })).toEqual("b") + it("ignores adding duplicate key values", () => { + const root = createRoot() + const instance$ = new Subject>() + const instanceFn = jest.fn((id: string) => of(id)) + const [instanceNode, keys] = subinstance( + root, + "keyName", + () => instance$, + instanceFn, + ) + root.run() - updates$.next({ - key: "a", - value: "new A value", - }) - expect(instanceNode.getValue({ keyName: "a" })).toEqual("new A value") - expect(instanceNode.getValue({ keyName: "b" })).toEqual("b") + instance$.next({ + type: "add", + key: "a", + }) + expect(instanceNode.getValue({ keyName: "a" })).toEqual("a") + expect(instanceFn).toHaveBeenCalledTimes(1) - instance$.next({ - type: "remove", - key: "a", - }) - expect(() => instanceNode.getValue({ keyName: "a" })).toThrow() - expect(instanceNode.getValue({ keyName: "b" })).toEqual("b") + const keysObserver = jest.fn() + keys.getState$().subscribe(keysObserver) + expect(keysObserver).toHaveBeenCalledTimes(1) - instance$.next({ - type: "remove", - key: "b", + instance$.next({ + type: "add", + key: "a", + }) + expect(instanceFn).toHaveBeenCalledTimes(1) + expect(keysObserver).toHaveBeenCalledTimes(1) + + instance$.next({ + type: "remove", + key: "a", + }) + expect(() => instanceNode.getValue({ keyName: "a" })).toThrow() + expect(keysObserver).toHaveBeenCalledTimes(2) + }) + + it("ignores removing key values that don't exist", () => { + const root = createRoot() + const instance$ = new Subject>() + const [instanceNode] = subinstance( + root, + "keyName", + () => instance$, + (id) => of(id), + ) + root.run() + + instance$.next({ + type: "add", + key: "a", + }) + expect(instanceNode.getValue({ keyName: "a" })).toEqual("a") + + instance$.next({ + type: "remove", + key: "a", + }) + expect(() => instanceNode.getValue({ keyName: "a" })).toThrow() + + instance$.next({ + type: "remove", + key: "a", + }) + expect(() => instanceNode.getValue({ keyName: "a" })).toThrow() + }) + + it("cleans up all instances when the parent dies", () => { + const root = createRoot() + const [instances, keys] = subinstance( + root, + "keyName", + () => + from(["a", "b"]).pipe( + map((key) => ({ + type: "add", + key, + })), + ), + (id) => of(id), + ) + const stop = root.run() + + expect(instances.getValue({ keyName: "a" })).toEqual("a") + expect(instances.getValue({ keyName: "b" })).toEqual("b") + expect([...(keys.getValue() as Set)]).toEqual(["a", "b"]) + + stop() + expect(() => instances.getValue({ keyName: "a" })).toThrow() + expect(() => instances.getValue({ keyName: "b" })).toThrow() + expect(() => [...(keys.getValue() as Set)]).toThrow() + }) + }) + + describe("key selector", () => { + it("can access values from its context", () => { + const root = createRoot() + const keys = substate(root, () => of(["a", "b"])) + const [_, activeKeys] = subinstance( + keys, + "keyName", + (ctx) => + from(ctx(keys)).pipe( + map((key) => ({ + type: "add", + key, + })), + ), + (id) => of(id), + ) + root.run() + + expect([...(activeKeys.getValue() as Set)]).toEqual(["a", "b"]) + }) + + it("can reference siblings", () => { + const root = createRoot() + // TODO can't do this if root is already running + // Maybe on that case, catch exception and retry activating on microtask? + const [_, activeKeys] = subinstance( + root, + "keyName", + (_, getObs$) => + getObs$(keys).pipe( + mergeAll(), + map((key) => ({ + type: "add", + key, + })), + ), + (id) => of(id), + ) + const keys = substate(root, () => of(["a", "b"])) + root.run() + + expect([...(activeKeys.getValue() as Set)]).toEqual(["a", "b"]) + }) + }) + + describe("value selector", () => { + it("can access values from its context", () => { + const root = createRoot() + const values = substate(root, () => of({ a: 1, b: 2 })) + const [instances] = subinstance( + values, + "keyName", + () => + from(["a", "b"] as const).pipe( + map((key) => ({ + type: "add", + key, + })), + ), + (id, ctx) => of(ctx(values)[id]), + ) + root.run() + + expect(instances.getValue({ keyName: "a" })).toEqual(1) + expect(instances.getValue({ keyName: "b" })).toEqual(2) + }) + + it("can reference siblings", () => { + const root = createRoot() + const [instances] = subinstance( + root, + "keyName", + () => + from(["a", "b"] as const).pipe( + map((key) => ({ + type: "add", + key, + })), + ), + (id, _, getObs$) => getObs$(values).pipe(map((values) => values[id])), + ) + const values = substate(root, () => of({ a: 1, b: 2 })) + root.run() + + expect(instances.getValue({ keyName: "a" })).toEqual(1) + expect(instances.getValue({ keyName: "b" })).toEqual(2) }) - expect(() => instanceNode.getValue({ keyName: "b" })).toThrow() }) }) diff --git a/packages/context-state2/src/subinstance.ts b/packages/context-state2/src/subinstance.ts index 56ac514..03fe8d2 100644 --- a/packages/context-state2/src/subinstance.ts +++ b/packages/context-state2/src/subinstance.ts @@ -1,5 +1,10 @@ -import { Observable, map, scan, startWith } from "rxjs" -import { createStateNode, getInternals, trackParentChanges } from "./internal" +import { Observable, filter, map, scan, startWith } from "rxjs" +import { + Wildcard, + createStateNode, + getInternals, + trackParentChanges, +} from "./internal" import { substate } from "./substate" import { CtxFn, @@ -32,31 +37,39 @@ export function subinstance( keySelector: CtxFn, K>, instanceObs: InstanceCtxFn, KV>, ): [StateNode>, StateNode, K>] { - const instanceKeys = substate( - parent, - (ctx, getObs, key) => { - const keys = Object.assign(new Set(), { - lastUpdate: null, - } as { - lastUpdate: InstanceUpdate | null - }) - return keySelector(ctx, getObs, key).pipe( - scan((acc, change) => { - acc.lastUpdate = change - if (change.type === "add") { - acc.add(change.key) - } else { - acc.delete(change.key) - } - return acc - }, keys), - startWith(keys), - ) - }, - () => false, - ) - const parentInternals = getInternals(parent) + if (parentInternals.keysOrder.includes(keyName)) { + throw new Error(`Key "${keyName}" is already being used by a parent node`) + } + const instanceKeys = substate(parent, (ctx, getObs, key) => { + const keys = Object.assign(new Set(), { + lastUpdate: null, + } as { + lastUpdate: InstanceUpdate | null + }) + return keySelector(ctx, getObs, key).pipe( + scan((acc, change) => { + acc.lastUpdate = change + if (change.type === "add") { + if (acc.has(change.key)) { + acc.lastUpdate = null + } else { + acc.add(change.key) + } + } else { + if (acc.has(change.key)) { + acc.delete(change.key) + } else { + acc.lastUpdate = null + } + } + return acc + }, keys), + filter((v) => v.lastUpdate !== null), + startWith(keys), + ) + }) + const result = createStateNode, R>( [...parentInternals.keysOrder, keyName], [parentInternals], @@ -80,6 +93,9 @@ export function subinstance( .pipe(map((v, i) => [v, i] as const)) .subscribe(([v, i]) => { if (i === 0) { + // TODO ackchyually, this can't happen because `instanceKeys` has startWith(new Set()) + // Something I don't like from this is that this also means that there's currently no way of doing one single update with all the changes + // Maybe change API to { type: 'add', keys: key[] }? And also change startWith for defaultStart for (let instanceKey of v) { result.addInstance({ ...key, @@ -118,8 +134,15 @@ export function subinstance( }, onActive() {}, onReset() {}, - onRemoved(_, storage) { + onRemoved(key, storage) { storage.value.unsubscribe() + const orderedKey = parentInternals.keysOrder.map((k) => key[k]) + const instancesToRemove = [ + ...result.getInstances([...orderedKey, Wildcard] as any), + ] + instancesToRemove.forEach((instance) => + result.removeInstance(instance.key), + ) }, }) diff --git a/packages/context-state2/src/substate.ts b/packages/context-state2/src/substate.ts index da72928..87ee2e9 100644 --- a/packages/context-state2/src/substate.ts +++ b/packages/context-state2/src/substate.ts @@ -7,6 +7,13 @@ import { type StateNode, } from "./types" +/** + * + * @param parent + * @param getState$ + * @param equalityFn TODO <- this equality function actually refers to the parent?! + * @returns + */ export const substate = ( parent: StateNode, getState$: CtxFn,