Continue working on README

packages/reactivity-core/README.md

@@ -379,27 +379,23 @@ npm install @conterra/reactivity-core
 
 ## Gotchas and tips
 
-### Avoid side effects in computed signals
+### Avoid cycles in computed signals
 
-Computed signals should use a side effect free function.
-Oftentimes, you cannot control how often the function is re-executed, because that depends on how often
-the dependencies of your functions change and when your signal is actually read (computation is lazy).
+Don't use the value of a computed signal during its own computation.
+The error will be obvious in a simple example, but it may also occur by accident when many objects or functions are involved.
 
 Example:
 
 ```ts
-let nonReactiveValue = 1;
-const reactiveValue = reactive(1);
+import { computed } from "@conterra/reactivity-core";
+
 const computedValue = computed(() => {
-    // This works, but it usually bad style for the reasons outlined above:
-    nonReactiveValue += 1;
-
-    // XXX
-    // This is outright forbidden and will result in a runtime error.
-    // You cannot modify a signal from inside a computed signal.
-    reactiveValue.value += 1;
-    return "whatever";
+    // Trivial example. This may happen through many layers of indirection in real world code.
+    let v = computedValue.value;
+    return v * 2;
 });
+
+console.log(computedValue.value); // throws "Cycle detected"
 ```
 
 ### Don't trigger an effect from within itself
@@ -410,6 +406,8 @@ However, you should take care not to produce a cycle.
 Example: this is okay (but could be replaced by a computed signal).
 
 ```ts
+import { reactive, effect } from "@conterra/reactivity-core";
+
 const v1 = reactive(0);
 const v2 = reactive(1);
 effect(() => {
@@ -421,6 +419,8 @@ effect(() => {
 Example: this is _not_ okay.
 
 ```ts
+import { reactive, effect } from "@conterra/reactivity-core";
+
 const v1 = reactive(0);
 effect(() => {
     // same as `v1.value = v1.value + 1`
@@ -456,6 +456,48 @@ The example above will not throw an error anymore because the _read_ to `v1` has
 
 ### Batching multiple updates
 
+Every update to a signal will usually trigger all watchers.
+This is not really a problem when using the default `watch()` or `effect()`, since multiple changes that follow _immediately_ after each other are grouped into a single notification, with a minimal delay.
+
+However, when using `syncEffect` or `syncWatch`, you will be triggered many times:
+
+```ts
+import { reactive, syncEffect } from "@conterra/reactivity-core";
+
+const count = reactive(0);
+syncEffect(() => {
+    console.log(count.value);
+});
+
+count.value += 1;
+count.value += 1;
+count.value += 1;
+count.value += 1;
+// Effect has executed 5 times
+```
+
+You can avoid this by grouping many updates into a single _batch_.
+Effects or watchers will not get notified until the batch is complete:
+
+```ts
+import { reactive, syncEffect, batch } from "@conterra/reactivity-core";
+
+const count = reactive(0);
+syncEffect(() => {
+    console.log(count.value);
+});
+
+batch(() => {
+    count.value += 1;
+    count.value += 1;
+    count.value += 1;
+    count.value += 1;
+});
+// Effect has executed only twice: one initial call and once after batch() as completed.
+```
+
+It is usually a good idea to surround a complex update operation with `batch()`.
+
 ### Sync vs async effect / watch
 
 ### Writing nonreactive code

packages/reactivity-core/Reactive.ts

@@ -15,7 +15,7 @@ export type AddWritableBrand<T> = AddBrand<T> & { [IS_WRITABLE_REACTIVE]: true }
  *
  * When the value changes, all users of that value (computed signals, effects, watchers)
  * are notified automatically.
- * 
+ *
  * @group Primitives
  */
 export interface ReadonlyReactive<T> {
@@ -40,16 +40,16 @@ export interface ReadonlyReactive<T> {
      */
     peek(): T;
 
-    /** 
+    /**
      * Same as `.value`.
-     * 
+     *
      * For compatibility with builtin JS constructs.
      **/
     toJSON(): T;
 
     /**
      * Formats `.value` as a string.
-     * 
+     *
      * For compatibility with builtin JS constructs.
      **/
     toString(): string;
@@ -60,7 +60,7 @@ export interface ReadonlyReactive<T> {
  *
  * The value stored in this object can be changed through assignment,
  * and all its users will be notified automatically.
- * 
+ *
  * @group Primitives
  */
 export interface Reactive<T> extends ReadonlyReactive<T> {
@@ -82,7 +82,7 @@ export interface Reactive<T> extends ReadonlyReactive<T> {
  * A signal that holds a value from an external source.
  *
  * Instances of this type are used to integrate "foreign" state into the reactivity system.
- * 
+ *
  * @group Primitives
  */
 export interface ExternalReactive<T> extends ReadonlyReactive<T> {

packages/reactivity-core/ReactiveImpl.ts

@@ -16,14 +16,14 @@ import {
 
 /**
  * A function that should return `true` if `a` and `b` are considered equal, `false` otherwise.
- * 
+ *
  * @group Primitives
  */
 export type EqualsFunc<T> = (a: T, b: T) => boolean;
 
 /**
  * Options that can be passed when creating a new signal.
- * 
+ *
  * @group Primitives
  */
 export interface ReactiveOptions<T> {
@@ -47,7 +47,7 @@ export interface ReactiveOptions<T> {
  * console.log(foo.value); // undefined
  * foo.value = 123;        // updates the current value
  * ```
- * 
+ *
  * @group Primitives
  */
 export function reactive<T>(): Reactive<T | undefined>;
@@ -62,7 +62,7 @@ export function reactive<T>(): Reactive<T | undefined>;
  * console.log(foo.value); // 123
  * foo.value = 456;        // updates the current value
  * ```
- * 
+ *
  * @group Primitives
  */
 export function reactive<T>(initialValue: T, options?: ReactiveOptions<T>): Reactive<T>;
@@ -94,7 +94,7 @@ export function reactive<T>(
  * foo.value = 2;
  * console.log(doubleFoo.value); // 4
  * ```
- * 
+ *
  * @group Primitives
  */
 export function computed<T>(compute: () => T, options?: ReactiveOptions<T>): ReadonlyReactive<T> {
@@ -131,7 +131,7 @@ export function computed<T>(compute: () => T, options?: ReactiveOptions<T>): Rea
  *
  * // later: unsubscribe from signal
  * ```
- * 
+ *
  * @group Primitives
  */
 export function external<T>(compute: () => T, options?: ReactiveOptions<T>): ExternalReactive<T> {
@@ -158,7 +158,8 @@ export function external<T>(compute: () => T, options?: ReactiveOptions<T>): Ext
         invalidateSignal.value;
         return rawUntracked(() => compute());
     }, options);
-    (externalReactive as RemoveBrand<typeof externalReactive> as ReactiveImpl<T>).trigger = invalidate;
+    (externalReactive as RemoveBrand<typeof externalReactive> as ReactiveImpl<T>).trigger =
+        invalidate;
     return externalReactive as ExternalReactive<T>;
 }
 
@@ -190,7 +191,7 @@ export function external<T>(compute: () => T, options?: ReactiveOptions<T>): Ext
  * });
  * // now the effect runs once
  * ```
- * 
+ *
  * @group Primitives
  */
 export function batch<T>(callback: () => T): T {
@@ -204,7 +205,7 @@ export function batch<T>(callback: () => T): T {
  * even if they occur inside a computed object or in an effect.
  *
  * `untracked` returns the value of `callback()`.
- * 
+ *
  * @group Primitives
  */
 export function untracked<T>(callback: () => T): T {
@@ -216,7 +217,7 @@ export function untracked<T>(callback: () => T): T {
  * if it is not reactive.
  *
  * The access to `.value` is tracked.
- * 
+ *
  * @group Primitives
  */
 export function getValue<T>(maybeReactive: ReadonlyReactive<T> | T) {
@@ -231,7 +232,7 @@ export function getValue<T>(maybeReactive: ReadonlyReactive<T> | T) {
  * if it is not reactive.
  *
  * The access to `.value` is _not_ tracked.
- * 
+ *
  * @group Primitives
  */
 export function peekValue<T>(maybeReactive: ReadonlyReactive<T> | T) {

packages/reactivity-core/TaskQueue.ts

@@ -1,3 +1,4 @@
+import { reportTaskError } from "./reportTaskError";
 import { CleanupHandle } from "./sync";
 
 type TaskFn = () => void;
@@ -55,13 +56,12 @@ export class TaskQueue {
         // register and unregister for every iteration otherwise node will not terminate
         // https://stackoverflow.com/a/61574326
         this.channel.port2.addEventListener("message", this.messageHandler);
-
         this.channel.port1.postMessage(""); // queue macro task
     }
 
     private runIteration() {
         this.channel.port2.removeEventListener("message", this.messageHandler);
-        
+
         // Swap arrays so that NEW tasks are not queued into the same array;
         // they will be handled in the next iteration.
         const tasks = this.queue;
@@ -74,9 +74,7 @@ export class TaskQueue {
             try {
                 task.fn();
             } catch (e) {
-                // This makes the error an unhandled rejection for lack of a better
-                // reporting mechanisms. Stupid idea?
-                Promise.reject(new Error(`Error in effect or watch callback`, { cause: e }));
+                reportTaskError(e);
             }
         }
     }

packages/reactivity-core/async.test.ts

@@ -1,6 +1,11 @@
-import { describe, expect, it, vi } from "vitest";
+import { afterEach, describe, expect, it, vi } from "vitest";
 import { reactive } from "./ReactiveImpl";
 import { effect, watch } from "./async";
+import * as report from "./reportTaskError";
+
+afterEach(() => {
+    vi.restoreAllMocks();
+});
 
 describe("effect", () => {
     it("re-executes the callback asynchronously", async () => {
@@ -70,6 +75,35 @@ describe("effect", () => {
         await waitForMacroTask();
         expect(spy).toHaveBeenCalledTimes(1); // not called again
     });
+
+    it("throws when a cycle is detected", async () => {
+        const v1 = reactive(0);
+        expect(() => {
+            effect(() => {
+                v1.value = v1.value + 1;
+            });
+        }).toThrowError(/Cycle detected/);
+    });
+
+    it("throws when a cycle is detected in a later execution", async () => {
+        const errorSpy = vi.spyOn(report, "reportTaskError").mockImplementation(() => {});
+
+        const v1 = reactive(0);
+        const trigger = reactive(false);
+        effect(() => {
+            if (!trigger.value) {
+                return;
+            }
+
+            v1.value = v1.value + 1;
+        });
+        expect(errorSpy).toHaveBeenCalledTimes(0);
+
+        trigger.value = true;
+        await waitForMacroTask();
+        expect(errorSpy).toHaveBeenCalledTimes(1);
+        expect(errorSpy.mock.lastCall![0]).toMatchInlineSnapshot(`[Error: Cycle detected]`);
+    });
 });
 
 describe("watch", () => {