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

Removed difference in behaviour of @preact/signals and @preact/signal… #387

5 changes: 5 additions & 0 deletions .changeset/tender-pianos-love.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@preact/signals": minor
---

Removed difference in behaviour between adapters, signals that use a JSX value will correctly re-render the whole component rather than attempting the JSX-Text optimization.
15 changes: 10 additions & 5 deletions packages/preact/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { options, Component } from "preact";
import { options, Component, isValidElement } from "preact";
XantreDev marked this conversation as resolved.
Show resolved Hide resolved
import { useRef, useMemo, useEffect } from "preact/hooks";
import {
signal,
Expand Down Expand Up @@ -71,7 +71,7 @@ function createUpdater(update: () => void) {
* A wrapper component that renders a Signal directly as a Text node.
* @todo: in Preact 11, just decorate Signal with `type:null`
*/
function Text(this: AugmentedComponent, { data }: { data: Signal }) {
function SignalValue(this: AugmentedComponent, { data }: { data: Signal }) {
// hasComputeds.add(this);

// Store the props.data signal in another signal so that
Expand All @@ -89,8 +89,13 @@ function Text(this: AugmentedComponent, { data }: { data: Signal }) {
}
}

// Replace this component's vdom updater with a direct text one:
this._updater!._callback = () => {
XantreDev marked this conversation as resolved.
Show resolved Hide resolved
if (isValidElement(s.peek()) || this.base?.nodeType !== 3) {
XantreDev marked this conversation as resolved.
Show resolved Hide resolved
this._updateFlags |= HAS_PENDING_UPDATE;
this.setState({});
return;
}

(this.base as Text).data = s.peek();
};
XantreDev marked this conversation as resolved.
Show resolved Hide resolved

Expand All @@ -103,11 +108,11 @@ function Text(this: AugmentedComponent, { data }: { data: Signal }) {

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

Object.defineProperties(Signal.prototype, {
constructor: { configurable: true, value: undefined },
type: { configurable: true, value: Text },
type: { configurable: true, value: SignalValue },
props: {
configurable: true,
get() {
Expand Down
135 changes: 110 additions & 25 deletions packages/preact/test/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,15 @@ describe("@preact/signals", () => {
});
});

describe("Text bindings", () => {
describe("SignalValue bindings", () => {
it("should render text without signals", () => {
render(<span>test</span>, scratch);
const span = scratch.firstChild;
const text = span?.firstChild;
expect(text).to.have.property("data", "test");
});

it("should render Signals as Text", () => {
it("should render Signals as SignalValue", () => {
const sig = signal("test");
render(<span>{sig}</span>, scratch);
const span = scratch.firstChild;
Expand All @@ -51,7 +51,7 @@ describe("@preact/signals", () => {
expect(text).to.have.property("data", "test");
});

it("should update Signal-based Text (no parent component)", () => {
it("should update Signal-based SignalValue (no parent component)", () => {
const sig = signal("test");
render(<span>{sig}</span>, scratch);

Expand All @@ -60,13 +60,13 @@ describe("@preact/signals", () => {

sig.value = "changed";

// should not remount/replace Text
// should not remount/replace SignalValue
expect(scratch.firstChild!.firstChild!).to.equal(text);
// should update the text in-place
expect(text).to.have.property("data", "changed");
});

it("should update Signal-based Text (in a parent component)", async () => {
it("should update Signal-based SignalValue (in a parent component)", async () => {
const sig = signal("test");
const spy = sinon.spy();
function App({ x }: { x: typeof sig }) {
Expand All @@ -81,7 +81,7 @@ describe("@preact/signals", () => {

sig.value = "changed";

// should not remount/replace Text
// should not remount/replace SignalValue
expect(scratch.firstChild!.firstChild!).to.equal(text);
// should update the text in-place
expect(text).to.have.property("data", "changed");
Expand All @@ -90,7 +90,7 @@ describe("@preact/signals", () => {
expect(spy).not.to.have.been.called;
});

it("should support swapping Signals in Text positions", async () => {
it("should support swapping Signals in SignalValue positions", async () => {
const sig = signal("test");
const spy = sinon.spy();
function App({ x }: { x: typeof sig }) {
Expand All @@ -108,7 +108,7 @@ describe("@preact/signals", () => {
expect(spy).to.have.been.called;
spy.resetHistory();

// should not remount/replace Text
// should not remount/replace SignalValue
expect(scratch.firstChild!.firstChild!).to.equal(text);
// should update the text in-place
expect(text).to.have.property("data", "different");
Expand All @@ -131,6 +131,85 @@ describe("@preact/signals", () => {
await sleep();
expect(spy).not.to.have.been.called;
});

it("should support rendering JSX in SignalValue positions", async () => {
const sig = signal(<span>test</span>);
function App({ x }: { x: typeof sig }) {
return <span>{x}</span>;
}

render(<App x={sig} />, scratch);

const text = scratch.firstChild!.firstChild!;

expect(text.textContent).to.equal("test");
expect(text).to.be.an.instanceOf(HTMLSpanElement);
expect(text).to.have.property("firstChild").that.is.an.instanceOf(Text);
});

it("JSX in SignalValue should be reactive", async () => {
const sig = signal(<span>test</span>);
const spy = sinon.spy();
function App({ x }: { x: typeof sig }) {
spy();
return <span>{x}</span>;
}

render(<App x={sig} />, scratch);
expect(spy).to.have.been.calledOnce;
spy.resetHistory();

const text = scratch.firstChild!.firstChild!;

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 = <div>a</div>;

expect(spy).not.to.have.been.calledOnce;

rerender();
scratch.firstChild!.firstChild!.textContent!.should.equal("a");
});

it("should support swapping between JSX and string in SignalValue positions", async () => {
const sig = signal<JSX.Element | string>(<span>test</span>);
function App({ x }: { x: typeof sig }) {
return <span>{x}</span>;
}

render(<App x={sig} />, scratch);

let text = scratch.firstChild!.firstChild!;

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();
text = scratch.firstChild!.firstChild!;
expect(text.nodeType).to.equal(Node.TEXT_NODE);
expect(text.textContent).to.equal("a");

sig.value = "b";
expect(text.textContent).to.equal("b");

sig.value = <div>c</div>;
rerender();
await sleep();
text = scratch.firstChild!.firstChild!;

expect(text).to.be.an.instanceOf(HTMLDivElement);
expect(text.textContent).to.equal("c");
sig.value = <span>d</span>;
rerender();
await sleep();

text = scratch.firstChild!.firstChild!;
expect(text).to.be.an.instanceOf(HTMLSpanElement);
expect(text.textContent).to.equal("d");
});
});

describe("Component bindings", () => {
Expand Down Expand Up @@ -418,38 +497,44 @@ describe("@preact/signals", () => {
});
});

describe('hooks mixed with signals', () => {
it('signals should not stop context from propagating', () => {
const ctx = createContext({ test: 'should-not-exist' });
describe("hooks mixed with signals", () => {
XantreDev marked this conversation as resolved.
Show resolved Hide resolved
it("signals should not stop context from propagating", () => {
const ctx = createContext({ test: "should-not-exist" });
let update: any;

function Provider(props: any) {
const [test, setTest] = useState('foo');
update = setTest
return (
<ctx.Provider value={{ test }}>{props.children}</ctx.Provider>
);
const [test, setTest] = useState("foo");
update = setTest;
return <ctx.Provider value={{ test }}>{props.children}</ctx.Provider>;
}

const s = signal('baz')
const s = signal("baz");
function Test() {
const value = useContext(ctx);
return <p>{value.test} {s.value}</p>
return (
<p>
{value.test} {s.value}
</p>
);
}

function App() {
return <Provider><Test /></Provider>
return (
<Provider>
<Test />
</Provider>
);
}

render(<App />, scratch);

expect(scratch.innerHTML).to.equal('<p>foo baz</p>')
expect(scratch.innerHTML).to.equal("<p>foo baz</p>");
act(() => {
update('bar')
})
expect(scratch.innerHTML).to.equal('<p>bar baz</p>')
})
})
update("bar");
});
expect(scratch.innerHTML).to.equal("<p>bar baz</p>");
});
});

describe("useSignalEffect()", () => {
it("should be invoked after commit", async () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/preact/test/ssr.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ describe("@preact/signals", () => {
expect(() => renderToString(<App />)).not.to.throw();
});

describe("Text bindings", () => {
describe("SignalValue bindings", () => {
it("should render strings", () => {
const s = signal("hello");
expect(renderToString(<p>{s}</p>)).to.equal(`<p>hello</p>`);
Expand Down
8 changes: 4 additions & 4 deletions packages/react/runtime/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,16 +119,16 @@ export function useSignals(): () => void {
}

/**
* A wrapper component that renders a Signal's value directly as a Text node.
* A wrapper component that renders a Signal's value directly as a Text node or JSX.
*/
function Text({ data }: { data: Signal }) {
function SignalValue({ data }: { data: Signal }) {
return data.value;
}

// Decorate Signals so React renders them as <Text> components.
// Decorate Signals so React renders them as <SignalValue> components.
Object.defineProperties(Signal.prototype, {
$$typeof: { configurable: true, value: ReactElemType },
type: { configurable: true, value: Text },
type: { configurable: true, value: SignalValue },
props: {
configurable: true,
get() {
Expand Down
34 changes: 27 additions & 7 deletions packages/react/test/browser/updates.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,15 @@ describe("@preact/signals-react updating", () => {
checkHangingAct();
});

describe("Text bindings", () => {
describe("SignalValue bindings", () => {
it("should render text without signals", async () => {
await render(<span>test</span>);
const span = scratch.firstChild;
const text = span?.firstChild;
expect(text).to.have.property("data", "test");
});

it("should render Signals as Text", async () => {
it("should render Signals as SignalValue", async () => {
const sig = signal("test");
await render(<span>{sig}</span>);
const span = scratch.firstChild;
Expand All @@ -74,7 +74,7 @@ describe("@preact/signals-react updating", () => {
expect(text).to.have.property("data", "test");
});

it("should render computed as Text", async () => {
it("should render computed as SignalValue", async () => {
const sig = signal("test");
const comp = computed(() => `${sig} ${sig}`);
await render(<span>{comp}</span>);
Expand All @@ -84,7 +84,7 @@ describe("@preact/signals-react updating", () => {
expect(text).to.have.property("data", "test test");
});

it("should update Signal-based Text (no parent component)", async () => {
it("should update Signal-based SignalValue (no parent component)", async () => {
const sig = signal("test");
await render(<span>{sig}</span>);

Expand All @@ -95,13 +95,13 @@ describe("@preact/signals-react updating", () => {
sig.value = "changed";
});

// should not remount/replace Text
// should not remount/replace SignalValue
expect(scratch.firstChild!.firstChild!).to.equal(text);
// should update the text in-place
expect(text).to.have.property("data", "changed");
});

it("should update Signal-based Text (in a parent component)", async () => {
it("should update Signal-based SignalValue (in a parent component)", async () => {
const sig = signal("test");
function App({ x }: { x: typeof sig }) {
return <span>{x}</span>;
Expand All @@ -115,11 +115,31 @@ describe("@preact/signals-react updating", () => {
sig.value = "changed";
});

// should not remount/replace Text
// should not remount/replace SignalValue
expect(scratch.firstChild!.firstChild!).to.equal(text);
// should update the text in-place
expect(text).to.have.property("data", "changed");
});

it("should work with JSX inside signal", async () => {
const sig = signal(<b>test</b>);
function App({ x }: { x: typeof sig }) {
return <span>{x}</span>;
}
await render(<App x={sig} />);

let text = scratch.firstChild!.firstChild!;
expect(text).to.be.instanceOf(HTMLElement);
expect(text.firstChild).to.have.property("data", "test");

await act(() => {
sig.value = <div>changed</div>;
});

text = scratch.firstChild!.firstChild!;
expect(text).to.be.instanceOf(HTMLDivElement);
expect(text.firstChild).to.have.property("data", "changed");
});
});

describe("Component bindings", () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/react/test/shared/mounting.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,13 @@ export function mountSignalsTests(
expect(html).to.equal("<span>test</span>");
});

it("should render Signals as Text", async () => {
it("should render Signals as SignalValue", async () => {
const sig = signal("test");
const html = await render(<span>{sig}</span>);
expect(html).to.equal("<span>test</span>");
});

it("should render computed as Text", async () => {
it("should render computed as SignalValue", async () => {
const sig = signal("test");
const comp = computed(() => `${sig} ${sig}`);
const html = await render(<span>{comp}</span>);
Expand Down