Skip to content

Commit

Permalink
Simplify test case a bit
Browse files Browse the repository at this point in the history
  • Loading branch information
andrewiggins committed Oct 17, 2023
1 parent bd3fe1a commit 694c605
Showing 1 changed file with 18 additions and 29 deletions.
47 changes: 18 additions & 29 deletions packages/react/runtime/test/useSignals.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -525,46 +525,37 @@ describe("useSignals", () => {
expect(scratch.innerHTML).to.equal("<div>Hello John</div>");
});

it.only("(unmanaged) should work with rerenders that update signals before async final cleanup", async () => {
it.only("(unmanaged) React 16 should work with rerenders that update signals before async final cleanup", async () => {
// Cursed/problematic ordering:
// 1. onClick callback
// 1a. call setState (queues sync work at end of event handler in React)
// 2b. await Promise.resolve();
// 3. React flushes sync callbacks and rerenders component
// 3a. Start useSignals effect & start batch
// 4. Resolve promises resumes and sets signal value
// 4a. In batch, so mark subscribers as needing update
// 5. useSignals finalCleanup runs
// 5a. finishEffect runs
// 5aa. endBatch and call subscribers with signal update
// 5ab. usSignals + useSyncExternalStore calls onChangeNotifyReact
// 5ac. React sync rerenders component
// 5ad. useSignals effect runs
// 5ae. finishEffect called again (evalContext == null but this == effectInstance)
// 1b. await Promise.resolve();
// 2. React flushes sync callbacks and rerenders component
// 2a. Start useSignals effect, set evalContext, & start batch
// 3. Resolve promises resumes and sets signal value
// 3a. In batch, so mark subscribers as needing update
// 4. useSignals finalCleanup runs
// 4a. endEffect runs and clears evalContext
// 4aa. endBatch and call subscribers with signal update
// 4ab. usSignals' useSyncExternalStore calls onChangeNotifyReact
// 4ac. React sync rerenders component
// 4ad. useSignals effect runs
// 4ae. finishEffect called again (evalContext == null but this == effectInstance)
// BOOM! Error thrown

let asyncOp: Promise<void>;
// const delay = (ms = 0) => new Promise(r => setTimeout(r, ms));
const delay = () => Promise.resolve();
async function testAsync(cb: () => any) {
asyncOp = delay().then(() => cb());
}

const count = signal(0);

function C() {
console.log("rendering C");

Check warning on line 549 in packages/react/runtime/test/useSignals.test.tsx

View workflow job for this annotation

GitHub Actions / tests

Unexpected console statement
useSignals();
const [loading, setLoading] = useState(false);

const onClick = () => {
const onClick = async () => {
setLoading(true);
console.log("onClick");

Check warning on line 555 in packages/react/runtime/test/useSignals.test.tsx

View workflow job for this annotation

GitHub Actions / tests

Unexpected console statement
testAsync(() => {
console.log("setSignal");
count.value += 1;
// setLoading(false);
});
await Promise.resolve();
console.log("setSignal");

Check warning on line 557 in packages/react/runtime/test/useSignals.test.tsx

View workflow job for this annotation

GitHub Actions / tests

Unexpected console statement
count.value += 1;
};

return (
Expand All @@ -587,9 +578,7 @@ describe("useSignals", () => {
`<p>0</p><button disabled="">loading...</button>`
);

console.log("wait start");
await asyncOp!;
console.log("wait finish");
// Do I need to do something here before this assertion?
expect(scratch.innerHTML).to.equal("<p>1</p><button>increment</button>");
});

Expand Down

0 comments on commit 694c605

Please sign in to comment.