From 57b2ec4716c03b961210b384d67c0ab64b711d21 Mon Sep 17 00:00:00 2001 From: Cody Bennett <23324155+CodyJasonBennett@users.noreply.github.com> Date: Mon, 12 Sep 2022 08:02:24 -0500 Subject: [PATCH 1/9] fix(react): track owners separately, create updaters per dispatcher --- packages/react/src/index.ts | 47 +++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 83b597683..34ba8519c 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -54,7 +54,6 @@ function createPropUpdater(props: any, prop: string, signal: Signal) { */ let finishUpdate: ReturnType | undefined; -const updaterForComponent = new WeakMap(); function setCurrentUpdater(updater?: Updater) { // end tracking for the current update: @@ -90,12 +89,27 @@ Object.defineProperties(Signal.prototype, { ref: { value: null }, }); +// Stores all tracked component Fibers (note: this is never cleared and will leak memory) +const componentQueue = new Set(); + // Track the current owner (roughly equiv to current vnode) -// let currentOwner: ReactOwner; -// Object.defineProperty(internals.ReactCurrentOwner, "current", { -// get() { return currentOwner; }, -// set(owner) { currentOwner = owner; }, -// }); +let currentOwner: ReactOwner; +Object.defineProperty(internals.ReactCurrentOwner, "current", { + get() { + return currentOwner; + }, + set(owner) { + currentOwner = owner; + if (currentOwner && !componentQueue.has(currentOwner)) + componentQueue.add(currentOwner); + }, +}); + +// Tracks component updaters per dispatcher +const dispatcherQueue = new WeakMap< + ReactDispatcher, + WeakMap +>(); // Track the current dispatcher (roughly equiv to current component impl) let lock = false; @@ -106,6 +120,12 @@ Object.defineProperty(internals.ReactCurrentDispatcher, "current", { return currentDispatcher; }, set(api) { + let updaterForComponent = dispatcherQueue.get(api); + if (!updaterForComponent) { + updaterForComponent = new WeakMap(); + dispatcherQueue.set(api, updaterForComponent); + } + currentDispatcher = api; if (lock) return; if (api && !isInvalidHookAccessor(api)) { @@ -114,13 +134,14 @@ Object.defineProperty(internals.ReactCurrentDispatcher, "current", { lock = true; const rerender = api.useReducer(UPDATE, {})[1]; lock = false; - const currentOwner = internals.ReactCurrentOwner.current; - let updater = updaterForComponent.get(currentOwner); - if (!updater) { - updater = createUpdater(rerender); - updaterForComponent.set(currentOwner, updater); - } - setCurrentUpdater(updater); + componentQueue.forEach(currentOwner => { + let updater = updaterForComponent!.get(currentOwner); + if (!updater) { + updater = createUpdater(rerender); + updaterForComponent!.set(currentOwner, updater); + } + setCurrentUpdater(updater); + }); } else { setCurrentUpdater(); } From 57a679d03be5b2f0708d5f679bc8f756fa8cc90b Mon Sep 17 00:00:00 2001 From: Cody Bennett <23324155+CodyJasonBennett@users.noreply.github.com> Date: Mon, 12 Sep 2022 17:34:04 -0500 Subject: [PATCH 2/9] fix(react): move updaterForComponent check inside api guard --- packages/react/src/index.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 34ba8519c..4b8eb9adb 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -120,15 +120,15 @@ Object.defineProperty(internals.ReactCurrentDispatcher, "current", { return currentDispatcher; }, set(api) { - let updaterForComponent = dispatcherQueue.get(api); - if (!updaterForComponent) { - updaterForComponent = new WeakMap(); - dispatcherQueue.set(api, updaterForComponent); - } - currentDispatcher = api; if (lock) return; if (api && !isInvalidHookAccessor(api)) { + let updaterForComponent = dispatcherQueue.get(api); + if (!updaterForComponent) { + updaterForComponent = new WeakMap(); + dispatcherQueue.set(api, updaterForComponent); + } + // prevent re-injecting useReducer when the Dispatcher // context changes to run the reducer callback: lock = true; From 402f5a6607e7aa849f723b6f2cf6f8615f5808d5 Mon Sep 17 00:00:00 2001 From: Cody Bennett <23324155+CodyJasonBennett@users.noreply.github.com> Date: Mon, 12 Sep 2022 21:20:32 -0500 Subject: [PATCH 3/9] refactor(react): don't create a component queue, track non-null component/dispatcher --- packages/react/src/index.ts | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 4b8eb9adb..b339c1227 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -89,10 +89,8 @@ Object.defineProperties(Signal.prototype, { ref: { value: null }, }); -// Stores all tracked component Fibers (note: this is never cleared and will leak memory) -const componentQueue = new Set(); - // Track the current owner (roughly equiv to current vnode) +let lastComponent: ReactOwner; let currentOwner: ReactOwner; Object.defineProperty(internals.ReactCurrentOwner, "current", { get() { @@ -100,20 +98,17 @@ Object.defineProperty(internals.ReactCurrentOwner, "current", { }, set(owner) { currentOwner = owner; - if (currentOwner && !componentQueue.has(currentOwner)) - componentQueue.add(currentOwner); + if (currentOwner) lastComponent = currentOwner; }, }); // Tracks component updaters per dispatcher -const dispatcherQueue = new WeakMap< - ReactDispatcher, - WeakMap ->(); +const updaterForComponent = new WeakMap(); // Track the current dispatcher (roughly equiv to current component impl) let lock = false; const UPDATE = () => ({}); +let lastDispatcher: ReactDispatcher; let currentDispatcher: ReactDispatcher; Object.defineProperty(internals.ReactCurrentDispatcher, "current", { get() { @@ -122,26 +117,19 @@ Object.defineProperty(internals.ReactCurrentDispatcher, "current", { set(api) { currentDispatcher = api; if (lock) return; - if (api && !isInvalidHookAccessor(api)) { - let updaterForComponent = dispatcherQueue.get(api); - if (!updaterForComponent) { - updaterForComponent = new WeakMap(); - dispatcherQueue.set(api, updaterForComponent); - } - + if (lastComponent && api && !isInvalidHookAccessor(api)) { // prevent re-injecting useReducer when the Dispatcher // context changes to run the reducer callback: lock = true; const rerender = api.useReducer(UPDATE, {})[1]; lock = false; - componentQueue.forEach(currentOwner => { - let updater = updaterForComponent!.get(currentOwner); - if (!updater) { - updater = createUpdater(rerender); - updaterForComponent!.set(currentOwner, updater); - } - setCurrentUpdater(updater); - }); + + let updater = updaterForComponent.get(lastComponent); + if (!updater || !lastDispatcher !== api) { + updater = createUpdater(rerender); + updaterForComponent.set(lastComponent, updater); + } + setCurrentUpdater(updater); } else { setCurrentUpdater(); } From 72a41cee0a24ada970f5e63c0ac07ace43aa07f6 Mon Sep 17 00:00:00 2001 From: Cody Bennett <23324155+CodyJasonBennett@users.noreply.github.com> Date: Mon, 12 Sep 2022 21:26:56 -0500 Subject: [PATCH 4/9] chore(react): cleanup --- packages/react/src/index.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index b339c1227..165a36e23 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -54,6 +54,7 @@ function createPropUpdater(props: any, prop: string, signal: Signal) { */ let finishUpdate: ReturnType | undefined; +const updaterForComponent = new WeakMap(); function setCurrentUpdater(updater?: Updater) { // end tracking for the current update: @@ -102,9 +103,6 @@ Object.defineProperty(internals.ReactCurrentOwner, "current", { }, }); -// Tracks component updaters per dispatcher -const updaterForComponent = new WeakMap(); - // Track the current dispatcher (roughly equiv to current component impl) let lock = false; const UPDATE = () => ({}); From 1050f72430365da37af68eddf672a8f13498c719 Mon Sep 17 00:00:00 2001 From: Cody Bennett <23324155+CodyJasonBennett@users.noreply.github.com> Date: Mon, 12 Sep 2022 21:33:11 -0500 Subject: [PATCH 5/9] chore(react): cleanup owner/component semantics --- packages/react/src/index.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 165a36e23..d0237cee2 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -91,7 +91,7 @@ Object.defineProperties(Signal.prototype, { }); // Track the current owner (roughly equiv to current vnode) -let lastComponent: ReactOwner; +let lastOwner: ReactOwner; let currentOwner: ReactOwner; Object.defineProperty(internals.ReactCurrentOwner, "current", { get() { @@ -99,7 +99,7 @@ Object.defineProperty(internals.ReactCurrentOwner, "current", { }, set(owner) { currentOwner = owner; - if (currentOwner) lastComponent = currentOwner; + if (currentOwner) lastOwner = currentOwner; }, }); @@ -115,17 +115,17 @@ Object.defineProperty(internals.ReactCurrentDispatcher, "current", { set(api) { currentDispatcher = api; if (lock) return; - if (lastComponent && api && !isInvalidHookAccessor(api)) { + if (lastOwner && api && !isInvalidHookAccessor(api)) { // prevent re-injecting useReducer when the Dispatcher // context changes to run the reducer callback: lock = true; const rerender = api.useReducer(UPDATE, {})[1]; lock = false; - let updater = updaterForComponent.get(lastComponent); + let updater = updaterForComponent.get(lastOwner); if (!updater || !lastDispatcher !== api) { updater = createUpdater(rerender); - updaterForComponent.set(lastComponent, updater); + updaterForComponent.set(lastOwner, updater); } setCurrentUpdater(updater); } else { From 3166861edaae7dae3a35d4bd67793d81120896c2 Mon Sep 17 00:00:00 2001 From: Cody Bennett <23324155+CodyJasonBennett@users.noreply.github.com> Date: Mon, 12 Sep 2022 21:39:07 -0500 Subject: [PATCH 6/9] fix(react): set lastDispatcher with updater --- packages/react/src/index.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index d0237cee2..cd08181b6 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -126,6 +126,7 @@ Object.defineProperty(internals.ReactCurrentDispatcher, "current", { if (!updater || !lastDispatcher !== api) { updater = createUpdater(rerender); updaterForComponent.set(lastOwner, updater); + lastDispatcher = api; } setCurrentUpdater(updater); } else { From cdd21e6786857f66f8486fcac7384c35776fceb6 Mon Sep 17 00:00:00 2001 From: Cody Bennett <23324155+CodyJasonBennett@users.noreply.github.com> Date: Mon, 12 Sep 2022 22:05:55 -0500 Subject: [PATCH 7/9] chore(react): cleanup owner/dispatcher types --- packages/react/src/index.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index cd08181b6..12c0549d2 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -91,8 +91,8 @@ Object.defineProperties(Signal.prototype, { }); // Track the current owner (roughly equiv to current vnode) -let lastOwner: ReactOwner; -let currentOwner: ReactOwner; +let lastOwner: ReactOwner | undefined; +let currentOwner: ReactOwner | null = null; Object.defineProperty(internals.ReactCurrentOwner, "current", { get() { return currentOwner; @@ -106,7 +106,7 @@ Object.defineProperty(internals.ReactCurrentOwner, "current", { // Track the current dispatcher (roughly equiv to current component impl) let lock = false; const UPDATE = () => ({}); -let lastDispatcher: ReactDispatcher; +let lastDispatcher: ReactDispatcher | undefined; let currentDispatcher: ReactDispatcher; Object.defineProperty(internals.ReactCurrentDispatcher, "current", { get() { @@ -123,7 +123,7 @@ Object.defineProperty(internals.ReactCurrentDispatcher, "current", { lock = false; let updater = updaterForComponent.get(lastOwner); - if (!updater || !lastDispatcher !== api) { + if (!updater || lastDispatcher !== api) { updater = createUpdater(rerender); updaterForComponent.set(lastOwner, updater); lastDispatcher = api; From 762d7d8556491eac0a364d4a254aa7d1a6058789 Mon Sep 17 00:00:00 2001 From: Cody Bennett <23324155+CodyJasonBennett@users.noreply.github.com> Date: Tue, 13 Sep 2022 04:53:34 -0500 Subject: [PATCH 8/9] fix(react): mutate updater in subsequent runs --- packages/react/src/index.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 12c0549d2..230a60214 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -106,7 +106,6 @@ Object.defineProperty(internals.ReactCurrentOwner, "current", { // Track the current dispatcher (roughly equiv to current component impl) let lock = false; const UPDATE = () => ({}); -let lastDispatcher: ReactDispatcher | undefined; let currentDispatcher: ReactDispatcher; Object.defineProperty(internals.ReactCurrentDispatcher, "current", { get() { @@ -123,10 +122,11 @@ Object.defineProperty(internals.ReactCurrentDispatcher, "current", { lock = false; let updater = updaterForComponent.get(lastOwner); - if (!updater || lastDispatcher !== api) { + if (!updater) { updater = createUpdater(rerender); updaterForComponent.set(lastOwner, updater); - lastDispatcher = api; + } else { + updater._updater = rerender; } setCurrentUpdater(updater); } else { From e3ead4ada0f85c90f17898a5d90006aaabd9ea13 Mon Sep 17 00:00:00 2001 From: Cody Bennett <23324155+CodyJasonBennett@users.noreply.github.com> Date: Tue, 13 Sep 2022 23:25:40 -0500 Subject: [PATCH 9/9] chore(tests): add react strictmode case --- packages/react/test/index.test.tsx | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/packages/react/test/index.test.tsx b/packages/react/test/index.test.tsx index 45f1df846..94d6ed27b 100644 --- a/packages/react/test/index.test.tsx +++ b/packages/react/test/index.test.tsx @@ -2,7 +2,7 @@ globalThis.IS_REACT_ACT_ENVIRONMENT = true; import { signal, useComputed } from "@preact/signals-react"; -import { createElement, useMemo } from "react"; +import { createElement, useMemo, memo, StrictMode } from "react"; import { createRoot, Root } from "react-dom/client"; import { act } from "react-dom/test-utils"; @@ -158,5 +158,26 @@ describe("@preact/signals-react", () => { }); expect(scratch.textContent).to.equal("bar"); }); + + it("should consistently rerender in strict mode", async () => { + const sig = signal(null!); + + const Test = memo(() =>

{sig.value}

); + const App = () => ( + + + + ); + + for (let i = 0; i < 3; i++) { + const value = `${i}`; + + act(() => { + sig.value = value; + render(); + }); + expect(scratch.textContent).to.equal(value); + } + }); }); });