Merge pull request #234 from preactjs/more-tests

Add tests for core & refactor addDependency for increased code coverage

Author: jviide

packages/core/src/index.ts

@@ -132,25 +132,21 @@ function addDependency(signal: Signal): Node | undefined {
 		}
 		return node;
 	} else if (node._version === -1) {
-		// `signal` is an existing dependency from a previous evaluation. Reuse the dependency
-		// node and move it to the front of the evaluation context's dependency list.
+		// `signal` is an existing dependency from a previous evaluation. Reuse it.
 		node._version = 0;
 
-		const head = evalContext._sources;
-		if (node !== head) {
-			const prev = node._prevSource;
-			const next = node._nextSource;
-			if (prev !== undefined) {
-				prev._nextSource = next;
-			}
-			if (next !== undefined) {
-				next._prevSource = prev;
-			}
-			if (head !== undefined) {
-				head._prevSource = node;
+		// If `node` is not already the current head of the dependency list (i.e.
+		// there is a previous node in the list), then make `node` the new head.
+		if (node._prevSource !== undefined) {
+			node._prevSource._nextSource = node._nextSource;
+			if (node._nextSource !== undefined) {
+				node._nextSource._prevSource = node._prevSource;
 			}
 			node._prevSource = undefined;
-			node._nextSource = head;
+			node._nextSource = evalContext._sources;
+			// evalCotext._sources must be !== undefined (and !== node), because
+			// `node` was originally pointing to some previous node.
+			evalContext._sources!._prevSource = node;
 			evalContext._sources = node;
 		}
 

packages/core/test/signal.test.tsx

@@ -29,6 +29,28 @@ describe("signal", () => {
 		expect(a + b).to.equal(3);
 	});
 
+	it("should notify other listeners of changes after one listener is disposed", () => {
+		const s = signal(0);
+		const spy1 = sinon.spy(() => s.value);
+		const spy2 = sinon.spy(() => s.value);
+		const spy3 = sinon.spy(() => s.value);
+
+		effect(spy1);
+		const dispose = effect(spy2);
+		effect(spy3);
+
+		expect(spy1).to.be.calledOnce;
+		expect(spy2).to.be.calledOnce;
+		expect(spy3).to.be.calledOnce;
+
+		dispose();
+
+		s.value = 1;
+		expect(spy1).to.be.calledTwice;
+		expect(spy2).to.be.calledOnce;
+		expect(spy3).to.be.calledTwice;
+	});
+
 	describe(".peek()", () => {
 		it("should get value", () => {
 			const s = signal(1);
@@ -865,7 +887,7 @@ describe("computed()", () => {
 		expect(() => effect(() => a.value)).to.not.throw();
 	});
 
-	it("should store failures and recompute only after a dependency changes", () => {
+	it("should store thrown errors and recompute only after a dependency changes", () => {
 		const a = signal(0);
 		const spy = sinon.spy(() => {
 			a.value;
@@ -880,6 +902,39 @@ describe("computed()", () => {
 		expect(spy).to.be.calledTwice;
 	});
 
+	it("should store thrown non-errors and recompute only after a dependency changes", () => {
+		const a = signal(0);
+		const spy = sinon.spy();
+		const c = computed(() => {
+			a.value;
+			spy();
+			throw undefined;
+		});
+
+		try {
+			c.value;
+			expect.fail();
+		} catch (err) {
+			expect(err).to.be.undefined;
+		}
+		try {
+			c.value;
+			expect.fail();
+		} catch (err) {
+			expect(err).to.be.undefined;
+		}
+		expect(spy).to.be.calledOnce;
+
+		a.value = 1;
+		try {
+			c.value;
+			expect.fail();
+		} catch (err) {
+			expect(err).to.be.undefined;
+		}
+		expect(spy).to.be.calledTwice;
+	});
+
 	it("should conditionally unsubscribe from signals", () => {
 		const a = signal("a");
 		const b = signal("b");
@@ -967,6 +1022,30 @@ describe("computed()", () => {
 		expect(c.value).to.equal(1);
 	});
 
+	it("should propagate notification to other listeners after one listener is disposed", () => {
+		const s = signal(0);
+		const c = computed(() => s.value);
+
+		const spy1 = sinon.spy(() => c.value);
+		const spy2 = sinon.spy(() => c.value);
+		const spy3 = sinon.spy(() => c.value);
+
+		effect(spy1);
+		const dispose = effect(spy2);
+		effect(spy3);
+
+		expect(spy1).to.be.calledOnce;
+		expect(spy2).to.be.calledOnce;
+		expect(spy3).to.be.calledOnce;
+
+		dispose();
+
+		s.value = 1;
+		expect(spy1).to.be.calledTwice;
+		expect(spy2).to.be.calledOnce;
+		expect(spy3).to.be.calledTwice;
+	});
+
 	it("should not recompute dependencies out of order", () => {
 		const a = signal(1);
 		const b = signal(1);
@@ -1042,6 +1121,19 @@ describe("computed()", () => {
 			expect(b.peek()).to.equal(2);
 		});
 
+		it("should detect simple dependency cycles", () => {
+			const a: Signal = computed(() => a.peek());
+			expect(() => a.peek()).to.throw(/Cycle detected/);
+		});
+
+		it("should detect deep dependency cycles", () => {
+			const a: Signal = computed(() => b.value);
+			const b: Signal = computed(() => c.value);
+			const c: Signal = computed(() => d.value);
+			const d: Signal = computed(() => a.peek());
+			expect(() => a.peek()).to.throw(/Cycle detected/);
+		});
+
 		it("should not make surrounding effect depend on the computed", () => {
 			const s = signal(1);
 			const c = computed(() => s.value);
@@ -1516,14 +1608,25 @@ describe("batch/transaction", () => {
 		expect(batch(() => 1)).to.equal(1);
 	});
 
-	it("should throw the error raised from the callback", () => {
+	it("should throw errors throws from the callback", () => {
 		expect(() =>
 			batch(() => {
 				throw Error("hello");
 			})
 		).to.throw("hello");
 	});
 
+	it("should throw non-errors thrown from the callback", () => {
+		try {
+			batch(() => {
+				throw undefined;
+			});
+			expect.fail();
+		} catch (err) {
+			expect(err).to.be.undefined;
+		}
+	});
+
 	it("should delay writes", () => {
 		const a = signal("a");
 		const b = signal("b");