Skip to content

Commit

Permalink
Raf aligned dom updates (#604)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>

* Update .changeset/ninety-beans-compare.md

* Skip changing the SignalValue notifier when it's an object (JSX)

---------

Co-authored-by: Ryan Christian <[email protected]>
  • Loading branch information
@JoviDeCroock @rschristian
JoviDeCroock and rschristian authored Oct 8, 2024
1 parent 5b6d891 commit fea3e8d
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 87 deletions.
16 changes: 16 additions & 0 deletions .changeset/ninety-beans-compare.md
View file
Original file line number Diff line number Diff line change
@@ -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.
75 changes: 32 additions & 43 deletions packages/preact/src/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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();
});
}, []);
Expand Down
96 changes: 52 additions & 44 deletions packages/preact/test/index.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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();

Expand All @@ -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");
Expand Down Expand Up @@ -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");
});

Expand All @@ -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();

Expand Down Expand Up @@ -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);
});
Expand All @@ -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");

Expand Down Expand Up @@ -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");

Expand Down Expand Up @@ -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,
Expand All @@ -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",
Expand All @@ -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++}>
Expand All @@ -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",
Expand All @@ -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");
});

Expand All @@ -771,8 +783,6 @@ describe("@preact/signals", () => {
render(<App />, scratch);
});

await afterFrame();

const child = scratch.firstElementChild;

expect(cleanup).not.to.have.been.called;
Expand All @@ -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);
});
Expand Down

0 comments on commit fea3e8d

Please sign in to comment.