Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: use watcher #266

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/loud-dolls-confess.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@preact/signals": major
"@preact/signals-react": major
---

Add useWatcher hook
33 changes: 20 additions & 13 deletions packages/preact/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,9 @@ function Text(this: AugmentedComponent, { data }: { data: Signal }) {

// Store the props.data signal in another signal so that
// passing a new signal reference re-runs the text computed:
const currentSignal = useSignal(data);
currentSignal.value = data;
const currentSignal = useWatcher(data);

const s = useMemo(() => {
const $s = useMemo(() => {
// mark the parent component as having computeds so it gets optimized
let v = this.__v;
while ((v = v.__!)) {
Expand All @@ -82,17 +81,16 @@ function Text(this: AugmentedComponent, { data }: { data: Signal }) {

// Replace this component's vdom updater with a direct text one:
this._updater!._callback = () => {
(this.base as Text).data = s.peek();
(this.base as Text).data = $s.peek();
};

return computed(() => {
let data = currentSignal.value;
let s = data.value;
const s = currentSignal.value.value;
return s === 0 ? 0 : s === true ? "" : s || "";
});
}, []);

return s.value;
return $s.value;
}
Text.displayName = "_st";

Expand All @@ -114,7 +112,7 @@ Object.defineProperties(Signal.prototype, {
/** Inject low-level property/attribute bindings for Signals into Preact's diff */
hook(OptionsTypes.DIFF, (old, vnode) => {
if (typeof vnode.type === "string") {
let signalProps: Record<string, any> | undefined;
let signalProps: typeof vnode.__np;

let props = vnode.props;
for (let i in props) {
Expand All @@ -136,7 +134,7 @@ hook(OptionsTypes.DIFF, (old, vnode) => {
hook(OptionsTypes.RENDER, (old, vnode) => {
setCurrentUpdater();

let updater;
let updater: Effect | undefined;

let component = vnode.__c;
if (component) {
Expand Down Expand Up @@ -187,15 +185,18 @@ hook(OptionsTypes.DIFFED, (old, vnode) => {
}
}
} else {
updaters = {};
dom._updaters = updaters;
dom._updaters = updaters = {};
}
for (let prop in props) {
let updater = updaters[prop];
let signal = props[prop];
if (updater === undefined) {
updater = createPropUpdater(dom, prop, signal, renderedProps);
updaters[prop] = updater;
updaters[prop] = updater = createPropUpdater(
dom,
prop,
signal,
renderedProps
);
} else {
updater._update(signal, renderedProps);
}
Expand Down Expand Up @@ -351,6 +352,12 @@ export function useSignalEffect(cb: () => void | (() => void)) {
}, []);
}

export function useWatcher<T>(value: T) {
const watcher = useSignal(value);
watcher.value = value;
return watcher as ReadonlySignal<T>;
}
Comment on lines +355 to +359
Copy link

@jamesarosen jamesarosen Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this implementation, I sometimes get the following warning:

react-dom.development.js:86 Warning: Cannot update a component (Foo) while rendering a different component (Bar). To locate the bad setState() call inside Bar, follow the stack trace as described in https://reactjs.org/link/setstate-in-render

The stack trace goes down as far as the useMemo call inside useSignal, then stops.

I changed it to this and the warning went away:

export default function useWatcher<T>(value: T): ReadonlySignal<T> {
  const watcher = useMemo<Signal<T>>(() => {
    return signal(value)
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [])

  useEffect(() => {
    watcher.value = value
  }, [value, watcher])
}

It's absolutely possible that the fault is entirely within my application and the change above papers over a real problem. It's also possible that setting watcher.value = value outside of a React hook is causing problems with React. I haven't narrowed that down yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should ignore it, its not something bad

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know how to fix it: wrap component in batch, so it will not try to rerender instantly. What do you think? @andrewiggins


/**
* @todo Determine which Reactive implementation we'll be using.
* @internal
Expand Down
2 changes: 1 addition & 1 deletion packages/preact/src/internal.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export interface VNode<P = any> extends preact.VNode<P> {
/** The DOM node for this VNode */
__e?: Element | Text;
/** Props that had Signal values before diffing (used after diffing to subscribe) */
__np?: Record<string, any> | null;
__np?: Record<string, Signal> | null;
}

export const enum OptionsTypes {
Expand Down
106 changes: 106 additions & 0 deletions packages/preact/test/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
computed,
useComputed,
useSignalEffect,
useWatcher,
Signal,
} from "@preact/signals";
import { createElement, createRef, render } from "preact";
Expand Down Expand Up @@ -353,6 +354,111 @@ describe("@preact/signals", () => {
});
});

describe("use watcher hook", () => {
it("should set the initial value of the checked property", () => {
function App({ checked = false }) {
const $checked = useWatcher(checked);
// @ts-ignore
return <input checked={$checked} />;
}

render(<App checked={true} />, scratch);
expect(scratch.firstChild).to.have.property("checked", true);
});

it("should update the checked property on change", () => {
function App({ checked = false }) {
const $checked = useWatcher(checked);
// @ts-ignore
return <input checked={$checked} />;
}

render(<App checked={true} />, scratch);
expect(scratch.firstChild).to.have.property("checked", true);

render(<App checked={false} />, scratch);
expect(scratch.firstChild).to.have.property("checked", false);
});

it("should update computed signal", () => {
function App({ value = 0 }) {
const $value = useWatcher(value);
const timesTwo = useComputed(() => $value.value * 2);
return <p>{timesTwo}</p>;
}

render(<App value={1} />, scratch);
expect(scratch.textContent).to.equal("2");

render(<App value={4} />, scratch);
expect(scratch.textContent).to.equal("8");
});

it("should not cascade rerenders", () => {
const spy = sinon.spy();
function App({ value = 0 }) {
const $value = useWatcher(value);
const timesTwo = useComputed(() => $value.value * 2);
spy();
return <p>{timesTwo.value}</p>;
}

render(<App value={1} />, scratch);
render(<App value={4} />, scratch);

expect(spy).to.be.calledTwice;
});

it("should update all silblings", () => {
function Test({ value }: { value: number }) {
const $value = useWatcher(value);
return <span>{$value.value}</span>;
}

function App({ value = 0 }) {
return (
<div>
<Test value={value} />
<Test value={value} />
</div>
);
}

const firstChild = () => scratch.firstChild?.firstChild;

render(<App value={1} />, scratch);
expect(firstChild()?.textContent).to.be.equal("1");
expect(firstChild()?.nextSibling?.textContent).to.be.equal("1");

render(<App value={4} />, scratch);
expect(firstChild()?.textContent).to.be.equal("4");
expect(firstChild()?.nextSibling?.textContent).to.be.equal("4");
});

it("should not rerender siblings", () => {
const spy = sinon.spy();
function Test({ value }: { value: number }) {
const $value = useWatcher(value);
spy();
return <span>{$value.value}</span>;
}

function App({ value = 0 }) {
return (
<div>
<Test value={value} />
<Test value={value} />
</div>
);
}

render(<App value={1} />, scratch);
render(<App value={4} />, scratch);

expect(spy).to.be.callCount(4);
});
});

describe("useSignalEffect()", () => {
it("should be invoked after commit", async () => {
const ref = createRef();
Expand Down
24 changes: 23 additions & 1 deletion packages/react/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ function WrapWithProxy(Component: FunctionComponent<any>) {
return WrappedComponent;
}

let renderDepth = 0;

/**
* A redux-like store whose store value is a positive 32bit integer (a 'version').
*
Expand All @@ -118,10 +120,24 @@ function createEffectStore() {
let version = 0;
let onChangeNotifyReact: (() => void) | undefined;

let unsubscribe = effect(function (this: Effect) {
const unsubscribe = effect(function (this: Effect) {
updater = this;
});

const oldStart = updater._start;

updater._start = function () {
renderDepth++;
const finish = oldStart.call(updater);
return function () {
finish();
renderDepth--;
};
};

updater._callback = function () {
// only notify React of state changes when not rendering in a component
if (renderDepth > 0) return;
version = (version + 1) | 0;
if (onChangeNotifyReact) onChangeNotifyReact();
};
Expand Down Expand Up @@ -235,3 +251,9 @@ export function useSignalEffect(cb: () => void | (() => void)) {
return effect(() => callback.current());
}, Empty);
}

export function useWatcher<T>(value: T) {
const watcher = useSignal(value);
watcher.value = value;
return watcher as ReadonlySignal<T>;
}
Loading