Raf aligned dom updates (#604)

* Raf aligned dom updates * Test approach leveraging our existing options.debounceRendering This all works fine, however it creates a discrepancy between browser usage and test usage which we want to avoid. The discrepancy is made clear in the failing tests. What we want to happen is for Preact to be first allowed to perform its render cycle, the effects that follow and only then should we clean up stragglers by performing the direct DOM updates. Currently what happens is that the ordering is "random" based on how the signals were registered. * options.requestAnimationFrame solves it * Improve changelog post Co-authored-by: Ryan Christian <33403762+rschristian@users.noreply.github.com> * Update .changeset/ninety-beans-compare.md * Skip changing the SignalValue notifier when it's an object (JSX) --------- Co-authored-by: Ryan Christian <33403762+rschristian@users.noreply.github.com>

Author: JoviDeCroock

.changeset/ninety-beans-compare.md

@@ -0,0 +1,16 @@
+---
+"@preact/signals": major
+---
+
+Defer all DOM updates by an animation frame, this should make it so
+that any previously synchronous DOM update will be instead delayed by an
+animation frame. This allows Preact to first perform its own render
+cycle and then our direct DOM updates to occur. These will now
+be performed in a batched way which is more performant as the browser
+is prepared to handle these during the animation frame.
+
+This does impact how Preact based signals are tested, when
+you perform a signal update, you'll need to wrap it in `act`. In a way
+this was always the case, as a signal update that resulted in
+a Preact state update would require it to be wrapped in `act`, but
+now this is the norm.

packages/preact/src/index.ts

@@ -79,32 +79,21 @@ function SignalValue(this: AugmentedComponent, { data }: { data: Signal }) {
 const currentSignal = useSignal(data);
 currentSignal.value = data;
 
-const s = useMemo(() => {
-// mark the parent component as having computeds so it gets optimized
-let v = this.__v;
-while ((v = v.__!)) {
-if (v.__c) {
-v.__c._updateFlags |= HAS_COMPUTEDS;
-break;
-}
-}
-
-this._updater!._callback = () => {
-if (isValidElement(s.peek()) || this.base?.nodeType !== 3) {
-this._updateFlags |= HAS_PENDING_UPDATE;
-this.setState({});
-return;
-}
+const s = useComputed(() => {
+let data = currentSignal.value;
+let s = data.value;
+return s === 0 ? 0 : s === true ? "" : s || "";
+});
 
-(this.base as Text).data = s.peek();
-};
+const isText = useComputed(() => {
+return !isValidElement(s.peek()) || this.base?.nodeType === 3;
+});
 
-return computed(() => {
-let data = currentSignal.value;
-let s = data.value;
-return s === 0 ? 0 : s === true ? "" : s || "";
-});
-}, []);
+useSignalEffect(() => {
+if (isText.value) {
+(this.base as Text).data = data.peek();
+}
+});
 
 return s.value;
 }
@@ -238,7 +227,9 @@ function createPropUpdater(
 changeSignal.value = newSignal;
 props = newProps;
 },
-_dispose: effect(() => {
+_dispose: effect(function (this: Effect) {
+if (!oldNotifyEffects) oldNotifyEffects = this._notify;
+this._notify = notifyEffects;
 const value = changeSignal.value.value;
 // If Preact just rendered this value, don't render it again:
 if (props[prop] === value) return;
@@ -358,28 +349,26 @@ export function useComputed<T>(compute: () => T) {
 return useMemo(() => computed<T>(() => $compute.current()), []);
 }
 
-let oldNotify: (this: Effect) => void,
-queue: Array<Effect> = [],
-isFlushing = false;
+let oldNotifyEffects: (this: Effect) => void,
+effectsQueue: Array<Effect> = [];
+
+const defer =
+typeof requestAnimationFrame === "undefined"
+? setTimeout
+: requestAnimationFrame;
 
-function flush() {
+function flushEffects() {
 batch(() => {
-let flushing = [...queue];
-isFlushing = false;
-queue.length = 0;
-for (let i = 0; i < flushing.length; i++) {
-oldNotify.call(flushing[i]);
+let inst: Effect | undefined;
+while ((inst = effectsQueue.shift())) {
+oldNotifyEffects.call(inst);
 }
 });
 }
 
-function notify(this: Effect) {
-queue.push(this);
-if (!isFlushing) {
-isFlushing = true;
-(typeof requestAnimationFrame === "undefined"
-? setTimeout
-: requestAnimationFrame)(flush);
+function notifyEffects(this: Effect) {
+if (effectsQueue.push(this) === 1) {
+(options.requestAnimationFrame || defer)(flushEffects);
 }
 }
 
@@ -389,8 +378,8 @@ export function useSignalEffect(cb: () => void | (() => void)) {
 
 useEffect(() => {
 return effect(function (this: Effect) {
-if (!oldNotify) oldNotify = this._notify;
-this._notify = notify;
+if (!oldNotifyEffects) oldNotifyEffects = this._notify;
+this._notify = notifyEffects;
 return callback.current();
 });
 }, []);

packages/preact/test/index.test.tsx

@@ -13,15 +13,6 @@ import { useContext, useRef, useState } from "preact/hooks";
 import { setupRerender, act } from "preact/test-utils";
 
 const sleep = (ms?: number) => new Promise(r => setTimeout(r, ms));
-const defer =
-typeof requestAnimationFrame === "undefined"
-? setTimeout
-: requestAnimationFrame;
-const afterFrame = () => {
-return new Promise(res => {
-defer(res);
-});
-};
 
 describe("@preact/signals", () => {
 let scratch: HTMLDivElement;
@@ -63,14 +54,16 @@ describe("@preact/signals", () => {
 expect(text).to.have.property("data", "test");
 });
 
-it("should update Signal-based SignalValue (no parent component)", () => {
+it("should update Signal-based SignalValue (no parent component)", async () => {
 const sig = signal("test");
 render(<span>{sig}</span>, scratch);
 
 const text = scratch.firstChild!.firstChild!;
 expect(text).to.have.property("data", "test");
 
-sig.value = "changed";
+act(() => {
+sig.value = "changed";
+});
 
 // should not remount/replace SignalValue
 expect(scratch.firstChild!.firstChild!).to.equal(text);
@@ -91,7 +84,9 @@ describe("@preact/signals", () => {
 const text = scratch.firstChild!.firstChild!;
 expect(text).to.have.property("data", "test");
 
-sig.value = "changed";
+act(() => {
+sig.value = "changed";
+});
 
 // should not remount/replace SignalValue
 expect(scratch.firstChild!.firstChild!).to.equal(text);
@@ -109,14 +104,19 @@ describe("@preact/signals", () => {
 spy();
 return <span>{x}</span>;
 }
-render(<App x={sig} />, scratch);
+
+act(() => {
+render(<App x={sig} />, scratch);
+});
 spy.resetHistory();
 
 const text = scratch.firstChild!.firstChild!;
 expect(text).to.have.property("data", "test");
 
 const sig2 = signal("different");
-render(<App x={sig2} />, scratch);
+act(() => {
+render(<App x={sig2} />, scratch);
+});
 expect(spy).to.have.been.called;
 spy.resetHistory();
 
@@ -128,14 +128,18 @@ describe("@preact/signals", () => {
 await sleep();
 expect(spy).not.to.have.been.called;
 
-sig.value = "changed old signal";
+act(() => {
+sig.value = "changed old signal";
+});
 
 await sleep();
 expect(spy).not.to.have.been.called;
 // the text should _not_ have changed:
 expect(text).to.have.property("data", "different");
 
-sig2.value = "changed";
+act(() => {
+sig2.value = "changed";
+});
 
 expect(scratch.firstChild!.firstChild!).to.equal(text);
 expect(text).to.have.property("data", "changed");
@@ -177,11 +181,10 @@ describe("@preact/signals", () => {
 expect(text).to.be.an.instanceOf(HTMLSpanElement);
 expect(text).to.have.property("firstChild").that.is.an.instanceOf(Text);
 
-sig.value = <div>a</div>;
-
+act(() => {
+sig.value = <div>a</div>;
+});
 expect(spy).not.to.have.been.calledOnce;
-
-rerender();
 scratch.firstChild!.firstChild!.textContent!.should.equal("a");
 });
 
@@ -198,23 +201,30 @@ describe("@preact/signals", () => {
 expect(text.textContent).to.equal("test");
 expect(text).to.be.an.instanceOf(HTMLSpanElement);
 expect(text).to.have.property("firstChild").that.is.an.instanceOf(Text);
-sig.value = "a";
-rerender();
+
+act(() => {
+sig.value = "a";
+});
 text = scratch.firstChild!.firstChild!;
 expect(text.nodeType).to.equal(Node.TEXT_NODE);
 expect(text.textContent).to.equal("a");
 
-sig.value = "b";
+act(() => {
+sig.value = "b";
+});
 expect(text.textContent).to.equal("b");
 
-sig.value = <div>c</div>;
-rerender();
+act(() => {
+sig.value = <div>c</div>;
+});
 await sleep();
 text = scratch.firstChild!.firstChild!;
 
 expect(text).to.be.an.instanceOf(HTMLDivElement);
 expect(text.textContent).to.equal("c");
-sig.value = <span>d</span>;
+act(() => {
+sig.value = <span>d</span>;
+});
 rerender();
 await sleep();
 
@@ -461,14 +471,16 @@ describe("@preact/signals", () => {
 expect(s.value).to.equal(true);
 });
 
-it("should update the checked property on change", () => {
+it("should update the checked property on change", async () => {
 const s = signal(true);
 // @ts-ignore
 render(<input checked={s} />, scratch);
 
 expect(scratch.firstChild).to.have.property("checked", true);
 
-s.value = false;
+act(() => {
+s.value = false;
+});
 
 expect(scratch.firstChild).to.have.property("checked", false);
 });
@@ -486,15 +498,19 @@ describe("@preact/signals", () => {
 
 expect(scratch.firstChild).to.have.property("value", "initial");
 
-s.value = "updated";
+act(() => {
+s.value = "updated";
+});
 
 expect(scratch.firstChild).to.have.property("value", "updated");
 
 // ensure the component was never re-rendered: (even after a tick)
 await sleep();
 expect(spy).not.to.have.been.called;
 
-s.value = "second update";
+act(() => {
+s.value = "second update";
+});
 
 expect(scratch.firstChild).to.have.property("value", "second update");
 
@@ -522,7 +538,9 @@ describe("@preact/signals", () => {
 await sleep();
 expect(spy).not.to.have.been.called;
 
-style.value = "left: 20px;";
+act(() => {
+style.value = "left: 20px;";
+});
 
 expect(div.style).to.have.property("left", "20px");
 
@@ -679,7 +697,6 @@ describe("@preact/signals", () => {
 });
 expect(scratch.textContent).to.equal("foo");
 // expect(spy).not.to.have.been.called;
-await afterFrame();
 expect(spy).to.have.been.calledOnceWith(
 "foo",
 scratch.firstElementChild,
@@ -690,11 +707,9 @@ describe("@preact/signals", () => {
 
 act(() => {
 sig.value = "bar";
-rerender();
 });
 
 expect(scratch.textContent).to.equal("bar");
-await afterFrame();
 
 expect(spy).to.have.been.calledOnceWith(
 "bar",
@@ -715,7 +730,9 @@ describe("@preact/signals", () => {
 const id = ref.current.getAttribute("data-render-id");
 const value = sig.value;
 spy(value, ref.current, id);
-return () => cleanup(value, ref.current, id);
+return () => {
+cleanup(value, ref.current, id);
+};
 });
 return (
 <p ref={ref} data-render-id={count++}>
@@ -728,7 +745,6 @@ describe("@preact/signals", () => {
 render(<App />, scratch);
 });
 
-await afterFrame();
 expect(cleanup).not.to.have.been.called;
 expect(spy).to.have.been.calledOnceWith(
 "foo",
@@ -739,16 +755,12 @@ describe("@preact/signals", () => {
 
 act(() => {
 sig.value = "bar";
-rerender();
 });
 
 expect(scratch.textContent).to.equal("bar");
-await afterFrame();
 
 const child = scratch.firstElementChild;
-
 expect(cleanup).to.have.been.calledOnceWith("foo", child, "0");
-
 expect(spy).to.have.been.calledOnceWith("bar", child, "1");
 });
 
@@ -771,8 +783,6 @@ describe("@preact/signals", () => {
 render(<App />, scratch);
 });
 
-await afterFrame();
-
 const child = scratch.firstElementChild;
 
 expect(cleanup).not.to.have.been.called;
@@ -783,8 +793,6 @@ describe("@preact/signals", () => {
 render(null, scratch);
 });
 
-await afterFrame();
-
 expect(spy).not.to.have.been.called;
 expect(cleanup).to.have.been.calledOnceWith("foo", child);
 });