Skip to content
Merged
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
5 changes: 5 additions & 0 deletions .changeset/restaged-effect-no-early-run.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@solidjs/signals": patch
---

A render effect created while an action holds a value it reads no longer runs its callback when a later pass re-stages it before the action lands (#3802). That run passed `undefined` as the value, because the effect had nothing committed yet, so compiled JSX bindings threw and halted reactivity. The effect now runs once, at the commit that lands the action, with the latest staged value.
4 changes: 3 additions & 1 deletion packages/signals/src/core/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,9 @@ export function recompute(el: Computed<any>, create: boolean = false): void {
el._flags & REACTIVE_SCREEN_READ &&
!(el as any)._modified &&
Object.is((el as any)._prevValue, value)
)
) &&
// A re-staging pass owes no run: the commit replays the effect (#3802).
el._pendingValue === NOT_PENDING
) {
(el as any)._modified = !el._x?._error;
// Reuse one bound runner per effect — runEffect no-ops on a stale
Expand Down
122 changes: 122 additions & 0 deletions packages/signals/tests/held-effect-restage-run-3802.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
// #3802: a render effect born held under an action, re-run by a write to a
// committed source, ran its callback with no committed value.

import { afterEach, expect, it, vi } from "vitest";
import {
action,
createMemo,
createOptimistic,
createRenderEffect,
createRoot,
createSignal,
flush
} from "../src/index.js";

afterEach(() => vi.restoreAllMocks());

async function settle() {
for (let i = 0; i < 8; i++) await Promise.resolve();
flush();
}

it("a re-staging pass of a born-held effect does not run it before the commit", async () => {
const error = vi.spyOn(console, "error").mockImplementation(() => {});
const [value, setValue] = createSignal(false);
const [mounted, setMounted] = createSignal(false);
const [title, setTitle] = createSignal("");
let resolve!: () => void;
const gate = new Promise<void>(r => (resolve = r));
const runs: [boolean, string][] = [];

const save = action(function* () {
setValue(true);
yield gate;
});

createRoot(() => {
const child = createMemo(() => {
if (!mounted()) return;
createMemo(() => value());
createRenderEffect(
() => ({ v: value(), t: title() }),
state => {
runs.push([state.v, state.t]);
}
);
});
createRenderEffect(child, () => {});
});
flush();

const pending = save();
await settle();
setMounted(true);
await settle();
setTitle("Updated");
await settle();

expect(error).not.toHaveBeenCalled();
expect(runs).toEqual([]);

resolve();
await pending;
await settle();

expect(error).not.toHaveBeenCalled();
expect(runs).toEqual([[true, "Updated"]]);
});

// The same effect re-derived by an optimistic write's lane rather than a
// mainline write: the lane's run had nothing committed to apply either.
it("a lane pass of a born-held effect does not run it with no committed value", async () => {
const error = vi.spyOn(console, "error").mockImplementation(() => {});
const [value, setValue] = createSignal(false);
const [mounted, setMounted] = createSignal(false);
const [guess, setGuess] = createOptimistic(0);
let resolveSave!: () => void;
let resolveVote!: () => void;
const runs: [boolean, number][] = [];

const save = action(function* () {
setValue(true);
yield new Promise<void>(r => (resolveSave = r));
});
const vote = action(function* () {
setGuess(5);
yield new Promise<void>(r => (resolveVote = r));
});

createRoot(() => {
const child = createMemo(() => {
if (!mounted()) return;
createMemo(() => value());
createRenderEffect(
() => ({ v: value(), g: guess() }),
state => {
runs.push([state.v, state.g]);
}
);
});
createRenderEffect(child, () => {});
});
flush();

const saving = save();
await settle();
setMounted(true);
await settle();
const voting = vote();
await settle();

expect(error).not.toHaveBeenCalled();

resolveVote();
await voting;
await settle();
resolveSave();
await saving;
await settle();

expect(error).not.toHaveBeenCalled();
expect(runs.at(-1)).toEqual([true, 0]);
});
68 changes: 68 additions & 0 deletions packages/web/test/held-action-memo-mount-3802.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/**
* @jsxImportSource @solidjs/web
* @vitest-environment jsdom
*/
import { afterEach, expect, test, vi } from "vitest";
import { Show, action, createMemo, createSignal, flush } from "solid-js";
import { render } from "../src/index.js";

function deferred() {
let resolve!: () => void;
const promise = new Promise<void>(r => (resolve = r));
return { promise, resolve };
}

async function settle() {
for (let i = 0; i < 8; i++) await Promise.resolve();
flush();
}

afterEach(() => vi.restoreAllMocks());

test("updating an attribute of a subtree mounted under a held action does not crash (#3802)", async () => {
const error = vi.spyOn(console, "error").mockImplementation(() => {});
const div = document.createElement("div");
const [value, setValue] = createSignal(false);
const [mounted, setMounted] = createSignal(false);
const [title, setTitle] = createSignal("");
const gate = deferred();

const save = action(function* () {
setValue(true);
yield gate.promise;
});

const dispose = render(
() => (
<main>
<Show when={mounted()}>
{_ => {
createMemo(() => value());
return <div class={String(value())} title={title()} />;
}}
</Show>
</main>
),
div
);
flush();

const pending = save();
await settle();
setMounted(true);
await settle();
setTitle("Updated");
await settle();

expect(error).not.toHaveBeenCalled();

gate.resolve();
await pending;
await settle();

expect(error).not.toHaveBeenCalled();
const el = div.querySelector("div")!;
expect(el.className).toBe("true");
expect(el.title).toBe("Updated");
dispose();
});
Loading