From 7f34c3b54ecde1c61d4ed1d4bb3200517e31762e Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 14 Apr 2023 16:38:41 -0700 Subject: [PATCH 01/26] Add reproduction of sync component rerenders causing failures --- packages/react/test/index.test.tsx | 31 ++++++++++++++++++++++++++++++ packages/react/test/utils.ts | 10 ++++++++++ 2 files changed, 41 insertions(+) diff --git a/packages/react/test/index.test.tsx b/packages/react/test/index.test.tsx index 5bd2d4d91..9fb16a836 100644 --- a/packages/react/test/index.test.tsx +++ b/packages/react/test/index.test.tsx @@ -16,6 +16,7 @@ import { memo, StrictMode, createRef, + useState, } from "react"; import { renderToStaticMarkup } from "react-dom/server"; @@ -26,11 +27,13 @@ import { checkHangingAct, isReact16, isProd, + consoleFormat, } from "./utils"; describe("@preact/signals-react", () => { let scratch: HTMLDivElement; let root: Root; + let errorSpy = sinon.spy(console, "error"); async function render(element: Parameters[0]) { await act(() => root.render(element)); @@ -40,12 +43,26 @@ describe("@preact/signals-react", () => { scratch = document.createElement("div"); document.body.appendChild(scratch); root = await createRoot(scratch); + errorSpy.resetHistory(); }); afterEach(async () => { checkHangingAct(); await act(() => root.unmount()); scratch.remove(); + + if (errorSpy.called) { + let message: string; + if (errorSpy.firstCall.args[0].toString().includes("%s")) { + message = consoleFormat(...errorSpy.firstCall.args); + } else { + message = errorSpy.firstCall.args.join(" "); + } + + expect.fail( + `Console.error was unexpectedly called with this message: \n${message}` + ); + } }); describe("Text bindings", () => { @@ -330,6 +347,20 @@ describe("@preact/signals-react", () => { `
-1${count.value * 2}
` ); }); + + it("should not fail when a component calls setState while rendering", async () => { + function App() { + const [state, setState] = useState(0); + if (state == 0) { + setState(1); + } + + return
{state}
; + } + + await render(); + expect(scratch.innerHTML).to.equal("
1
"); + }); }); describe("useSignal()", () => { diff --git a/packages/react/test/utils.ts b/packages/react/test/utils.ts index 2ac8645e1..d18aa3c21 100644 --- a/packages/react/test/utils.ts +++ b/packages/react/test/utils.ts @@ -69,3 +69,13 @@ export const act = process.env.NODE_ENV === "production" ? (prodActShim as typeof realAct) : realAct; + +/** + * `console.log` supports formatting strings with `%s` for string substitutions. + * This function accepts a string and additional arguments of values and returns + * a string with the values substituted in. + */ +export function consoleFormat(str: string, ...values: unknown[]): string { + let idx = 0; + return str.replace(/%s/g, () => String(values[idx++])); +} From 828e70130876e114ee414c6c20ec0ec6914e2f03 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 14 Apr 2023 16:39:03 -0700 Subject: [PATCH 02/26] Temporarily skip the renderToStaticMarkup test for now. Will address later --- packages/react/test/index.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/test/index.test.tsx b/packages/react/test/index.test.tsx index 9fb16a836..ebf5d06fc 100644 --- a/packages/react/test/index.test.tsx +++ b/packages/react/test/index.test.tsx @@ -277,7 +277,7 @@ describe("@preact/signals-react", () => { } }); - it("should render static markup of a component", async () => { + it.skip("should render static markup of a component", async () => { const count = signal(0); const Test = () => { From 68d292b9f039d77f11ac63d646a4c6612eeecc7f Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 14 Apr 2023 17:06:31 -0700 Subject: [PATCH 03/26] Update test to call unmount to do actual React unmount --- packages/react/test/index.test.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/react/test/index.test.tsx b/packages/react/test/index.test.tsx index ebf5d06fc..052405256 100644 --- a/packages/react/test/index.test.tsx +++ b/packages/react/test/index.test.tsx @@ -510,7 +510,9 @@ describe("@preact/signals-react", () => { expect(spy).to.have.been.calledOnceWith("foo", child); spy.resetHistory(); - await render(null); + await act(() => { + root.unmount(); + }); expect(scratch.innerHTML).to.equal(""); expect(spy).not.to.have.been.called; From 77d7298b4cce83c2ae69084f93bbd43f86dc2947 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 14 Apr 2023 17:08:49 -0700 Subject: [PATCH 04/26] Update component entering check to work for react 18 prod bundle --- packages/react/src/index.ts | 132 +++++++++++++++++++++++++++++------- 1 file changed, 109 insertions(+), 23 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index a172edb2a..d7eef5fc6 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -177,42 +177,40 @@ Object.defineProperty(ReactInternals.ReactCurrentDispatcher, "current", { const currentDispatcherType = getDispatcherType(currentDispatcher); const nextDispatcherType = getDispatcherType(nextDispatcher); - // We are entering a component render if the current dispatcher is the - // ContextOnlyDispatcher and the next dispatcher is a valid dispatcher. - const isEnteringComponentRender = - currentDispatcherType === ContextOnlyDispatcherType && - nextDispatcherType === ValidDispatcherType; - - // We are exiting a component render if the current dispatcher is a valid - // dispatcher and the next dispatcher is the ContextOnlyDispatcher. - const isExitingComponentRender = - currentDispatcherType === ValidDispatcherType && - nextDispatcherType === ContextOnlyDispatcherType; - // Update the current dispatcher now so the hooks inside of the // useSyncExternalStore shim get the right dispatcher. currentDispatcher = nextDispatcher; - if (isEnteringComponentRender) { + if (isEnteringComponentRender(currentDispatcherType, nextDispatcherType)) { lock = true; const store = usePreactSignalStore(nextDispatcher); lock = false; setCurrentUpdater(store.updater); - } else if (isExitingComponentRender) { + } else if ( + isExitingComponentRender(currentDispatcherType, nextDispatcherType) + ) { setCurrentUpdater(); } }, }); -const ValidDispatcherType = 0; -const ContextOnlyDispatcherType = 1; -const ErroringDispatcherType = 2; +const ContextOnlyDispatcherType = 1 << 0; +const ErroringDispatcherType = 1 << 1; +const MountDispatcherType = 1 << 2; +const UpdateDispatcherType = 1 << 3; +const ValidDispatcherType = MountDispatcherType | UpdateDispatcherType; +type DispatcherType = + | typeof ContextOnlyDispatcherType + | typeof ErroringDispatcherType + | typeof MountDispatcherType + | typeof UpdateDispatcherType + | typeof ValidDispatcherType; // We inject a useSyncExternalStore into every function component via // CurrentDispatcher. This prevents injecting into anything other than a // function component render. -const dispatcherTypeCache = new Map(); -function getDispatcherType(dispatcher: ReactDispatcher | null): number { +const dispatcherTypeCache = new Map(); +function getDispatcherType(dispatcher: ReactDispatcher | null): DispatcherType { // Treat null the same as the ContextOnlyDispatcher. if (!dispatcher) return ContextOnlyDispatcherType; @@ -223,20 +221,108 @@ function getDispatcherType(dispatcher: ReactDispatcher | null): number { // that takes no arguments and throws and error. Check the number of arguments // for this dispatcher's useCallback implementation to determine if it is a // ContextOnlyDispatcher. All other dispatchers, erroring or not, define - // functions with arguments and so fail this check. - let type: number; + // functions with arguments and so fail this check. For these dispatchers, we + // check the implementation for signifiers indicating what kind of dispatcher + // it is. + let type: DispatcherType; + const useCallbackImpl = dispatcher.useCallback.toString(); if (dispatcher.useCallback.length < 2) { type = ContextOnlyDispatcherType; - } else if (/Invalid/.test(dispatcher.useCallback as any)) { + } else if (/Invalid/.test(useCallbackImpl)) { type = ErroringDispatcherType; + } else if (/\[0\]/.test(useCallbackImpl) && /\[1\]/.test(useCallbackImpl)) { + // The mount and update dispatchers have a different implementation for the + // useCallback hook. The mount dispatcher stores the arguments to + // useCallback as an array on the state of the Fiber. The update dispatcher + // reads the values of the array using array indices (e.g. `[0]` and `[1]`). + // So we'll check for those indices to determine which dispatcher we are + // using. + type = UpdateDispatcherType; } else { - type = ValidDispatcherType; + type = MountDispatcherType; } dispatcherTypeCache.set(dispatcher, type); return type; } +function isEnteringComponentRender( + currentDispatcherType: DispatcherType, + nextDispatcherType: DispatcherType +): boolean { + if ( + currentDispatcherType & ContextOnlyDispatcherType && + nextDispatcherType & ValidDispatcherType + ) { + // ## First mount or update (ContextOnlyDispatcher -> ValidDispatcher (Mount or Update)) + // + // If the current dispatcher is the ContextOnlyDispatcher and the next + // dispatcher is a valid dispatcher, we are entering a component render. + return true; + } else if ( + (currentDispatcherType & MountDispatcherType && + nextDispatcherType & UpdateDispatcherType) || + (currentDispatcherType & UpdateDispatcherType && + nextDispatcherType & UpdateDispatcherType) + ) { + // ## Sync rerendering on mount (Mount -> Update) + // + // If we are transitioning from the mount dispatcher to an update + // dispatcher (i.e. HooksDispatcherOnMount to HooksDispatcherOnRerender), + // then this component is rerendering due to calling setState inside of + // its function body. We are re-entering a component's render method and + // so we should re-invoke our hooks. + // + // There is no known transition from HooksDispatcherOnMount to + // HooksDispatcherOnUpdate so the assumption that going from Mount to + // Update is a sync rerender is okay. + // + // ## Sync rerendering on update (Update -> Update) + // + // If we are transitioning from an update dispatcher to another update + // dispatcher (e.g. could be from HooksDispatcherOnUpdate to + // HooksDispatcherOnRerender or from HooksDispatcherOnRerender to + // HooksDispatcherOnRerender again), then this component is rerendering + // due to calling setState inside of its function body. We are re-entering + // a component's render method and so we should re-invoke our hooks. + // + // There is no known transition from HooksDispatcherOnUpdate to + // HooksDispatcherOnUpdate so the assumption that going from Update to + // Update is a sync rerender is okay. + return true; + } else { + // ## Resuming suspended mount edge case (Update -> Mount) + // + // If we are transitioning from the update dispatcher to the mount + // dispatcher, then this component is using the `use` hook and is resuming + // from a mount. We should not re-invoke our hooks in this situation since + // we are not entering a new component render, but instead continuing a + // previous render. + // + // ## Other transitions (Mount -> Mount, any transition in and out of invalid dispatchers) + // + // There is no known transition from HooksDispatcherOnMount to another mount + // dispatcher so we default to not triggering a re-enter of the component. + // Further transition in and out of invalid dispatchers does not indicate a + // new component render. + return false; + } +} + +/** + * We are exiting a component render if the current dispatcher is a valid + * dispatcher and the next dispatcher is the ContextOnlyDispatcher. + */ +function isExitingComponentRender( + currentDispatcherType: DispatcherType, + nextDispatcherType: DispatcherType +): boolean { + return Boolean( + currentDispatcherType & ValidDispatcherType && + nextDispatcherType & ContextOnlyDispatcherType + ); +} + function WrapJsx(jsx: T): T { if (typeof jsx !== "function") return jsx; From 6cd9ccf3a9255c04b32d6f96b3976860b3c0809e Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 14 Apr 2023 18:10:59 -0700 Subject: [PATCH 05/26] Update logic to explicitly discover rerender dispatcher --- packages/react/src/index.ts | 73 ++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 37 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index d7eef5fc6..bbbb9498e 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -198,7 +198,9 @@ const ContextOnlyDispatcherType = 1 << 0; const ErroringDispatcherType = 1 << 1; const MountDispatcherType = 1 << 2; const UpdateDispatcherType = 1 << 3; -const ValidDispatcherType = MountDispatcherType | UpdateDispatcherType; +const RerenderDispatcherType = 1 << 4; +const ValidDispatcherType = + MountDispatcherType | UpdateDispatcherType | RerenderDispatcherType; type DispatcherType = | typeof ContextOnlyDispatcherType | typeof ErroringDispatcherType @@ -235,9 +237,18 @@ function getDispatcherType(dispatcher: ReactDispatcher | null): DispatcherType { // useCallback hook. The mount dispatcher stores the arguments to // useCallback as an array on the state of the Fiber. The update dispatcher // reads the values of the array using array indices (e.g. `[0]` and `[1]`). - // So we'll check for those indices to determine which dispatcher we are - // using. - type = UpdateDispatcherType; + // So we'll check for those indices to determine if we are in a mount or + // update-like dispatcher. + + // The update and rerender dispatchers have different implementations for + // useReducer. We'll check it's implementation to determine if this is the + // rerender or update dispatcher. + let useReducerImpl = dispatcher.useReducer.toString(); + if (/return\s*\[[a-z]+,/.test(useReducerImpl)) { + type = RerenderDispatcherType; + } else { + type = UpdateDispatcherType; + } } else { type = MountDispatcherType; } @@ -259,36 +270,18 @@ function isEnteringComponentRender( // If the current dispatcher is the ContextOnlyDispatcher and the next // dispatcher is a valid dispatcher, we are entering a component render. return true; - } else if ( - (currentDispatcherType & MountDispatcherType && - nextDispatcherType & UpdateDispatcherType) || - (currentDispatcherType & UpdateDispatcherType && - nextDispatcherType & UpdateDispatcherType) - ) { - // ## Sync rerendering on mount (Mount -> Update) - // - // If we are transitioning from the mount dispatcher to an update - // dispatcher (i.e. HooksDispatcherOnMount to HooksDispatcherOnRerender), - // then this component is rerendering due to calling setState inside of - // its function body. We are re-entering a component's render method and - // so we should re-invoke our hooks. + } else if (nextDispatcherType & RerenderDispatcherType) { + // Any transition into the rerender dispatcher is a rerender is the + // beginning of a component render, so we should invoke our hooks. Details + // below. // - // There is no known transition from HooksDispatcherOnMount to - // HooksDispatcherOnUpdate so the assumption that going from Mount to - // Update is a sync rerender is okay. + // ## In-place rerendering on mount (Mount -> Rerender) // - // ## Sync rerendering on update (Update -> Update) - // - // If we are transitioning from an update dispatcher to another update - // dispatcher (e.g. could be from HooksDispatcherOnUpdate to - // HooksDispatcherOnRerender or from HooksDispatcherOnRerender to - // HooksDispatcherOnRerender again), then this component is rerendering - // due to calling setState inside of its function body. We are re-entering - // a component's render method and so we should re-invoke our hooks. - // - // There is no known transition from HooksDispatcherOnUpdate to - // HooksDispatcherOnUpdate so the assumption that going from Update to - // Update is a sync rerender is okay. + // If we are transitioning from the mount, update, or rerender dispatcher to the rerender + // dispatcher (e.g. HooksDispatcherOnMount to HooksDispatcherOnRerender), + // then this component is rerendering due to calling setState inside of its + // function body. We are re-entering a component's render method and so we + // should re-invoke our hooks. return true; } else { // ## Resuming suspended mount edge case (Update -> Mount) @@ -299,12 +292,18 @@ function isEnteringComponentRender( // we are not entering a new component render, but instead continuing a // previous render. // - // ## Other transitions (Mount -> Mount, any transition in and out of invalid dispatchers) + // ## Other transitions + // + // For example, Mount -> Mount, Update -> Update, Mount -> Update, any + // transition in and out of invalid dispatchers. // - // There is no known transition from HooksDispatcherOnMount to another mount - // dispatcher so we default to not triggering a re-enter of the component. - // Further transition in and out of invalid dispatchers does not indicate a - // new component render. + // There is no known transition for the following transitions so we default + // to not triggering a re-enter of the component. + // - HooksDispatcherOnMount -> HooksDispatcherOnMount + // - HooksDispatcherOnMount -> HooksDispatcherOnUpdate + // - HooksDispatcherOnUpdate -> HooksDispatcherOnUpdate + // - Any transition in and out of invalid dispatchers does not indicate a + // new component render. return false; } } From 22614478e64064f1756e00a7019b0fac01f56161 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 14 Apr 2023 18:24:57 -0700 Subject: [PATCH 06/26] Ignore timeout errors logged to console --- packages/react/test/index.test.tsx | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/react/test/index.test.tsx b/packages/react/test/index.test.tsx index 052405256..56049a516 100644 --- a/packages/react/test/index.test.tsx +++ b/packages/react/test/index.test.tsx @@ -59,9 +59,12 @@ describe("@preact/signals-react", () => { message = errorSpy.firstCall.args.join(" "); } - expect.fail( - `Console.error was unexpectedly called with this message: \n${message}` - ); + // Ignore errors for timeouts of tests that often happen while debugging + if (!message.includes("async tests and hooks,")) { + expect.fail( + `Console.error was unexpectedly called with this message: \n${message}` + ); + } } }); From 1a4e5e639f8e65c9b492db731a8bfce7964da95a Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 14 Apr 2023 18:43:31 -0700 Subject: [PATCH 07/26] Explicitly skip over transitions to and from development warning dispatchers --- packages/react/src/index.ts | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index bbbb9498e..21ba5051d 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -143,8 +143,8 @@ function usePreactSignalStore(nextDispatcher: ReactDispatcher): EffectStore { // // Some edge cases to be aware of: // - In development, useReducer, useState, and useMemo changes the dispatcher to -// a different erroring dispatcher before invoking the reducer and resets it -// right after. +// a different warning dispatcher (not ContextOnlyDispatcher) before invoking +// the reducer and resets it right after. // // The useSyncExternalStore shim will use some of these hooks when we invoke // it while entering a component render. We need to prevent this dispatcher @@ -195,7 +195,7 @@ Object.defineProperty(ReactInternals.ReactCurrentDispatcher, "current", { }); const ContextOnlyDispatcherType = 1 << 0; -const ErroringDispatcherType = 1 << 1; +const WarningDispatcherType = 1 << 1; const MountDispatcherType = 1 << 2; const UpdateDispatcherType = 1 << 3; const RerenderDispatcherType = 1 << 4; @@ -203,7 +203,7 @@ const ValidDispatcherType = MountDispatcherType | UpdateDispatcherType | RerenderDispatcherType; type DispatcherType = | typeof ContextOnlyDispatcherType - | typeof ErroringDispatcherType + | typeof WarningDispatcherType | typeof MountDispatcherType | typeof UpdateDispatcherType | typeof ValidDispatcherType; @@ -222,7 +222,7 @@ function getDispatcherType(dispatcher: ReactDispatcher | null): DispatcherType { // The ContextOnlyDispatcher sets all the hook implementations to a function // that takes no arguments and throws and error. Check the number of arguments // for this dispatcher's useCallback implementation to determine if it is a - // ContextOnlyDispatcher. All other dispatchers, erroring or not, define + // ContextOnlyDispatcher. All other dispatchers, warning or not, define // functions with arguments and so fail this check. For these dispatchers, we // check the implementation for signifiers indicating what kind of dispatcher // it is. @@ -231,8 +231,11 @@ function getDispatcherType(dispatcher: ReactDispatcher | null): DispatcherType { if (dispatcher.useCallback.length < 2) { type = ContextOnlyDispatcherType; } else if (/Invalid/.test(useCallbackImpl)) { - type = ErroringDispatcherType; - } else if (/\[0\]/.test(useCallbackImpl) && /\[1\]/.test(useCallbackImpl)) { + type = WarningDispatcherType; + } else if ( + /updateCallback/.test(useCallbackImpl) || + (/\[0\]/.test(useCallbackImpl) && /\[1\]/.test(useCallbackImpl)) + ) { // The mount and update dispatchers have a different implementation for the // useCallback hook. The mount dispatcher stores the arguments to // useCallback as an array on the state of the Fiber. The update dispatcher @@ -244,7 +247,10 @@ function getDispatcherType(dispatcher: ReactDispatcher | null): DispatcherType { // useReducer. We'll check it's implementation to determine if this is the // rerender or update dispatcher. let useReducerImpl = dispatcher.useReducer.toString(); - if (/return\s*\[[a-z]+,/.test(useReducerImpl)) { + if ( + /rerenderReducer/.test(useReducerImpl) || + /return\s*\[[a-z]+,/.test(useReducerImpl) + ) { type = RerenderDispatcherType; } else { type = UpdateDispatcherType; @@ -265,11 +271,22 @@ function isEnteringComponentRender( currentDispatcherType & ContextOnlyDispatcherType && nextDispatcherType & ValidDispatcherType ) { - // ## First mount or update (ContextOnlyDispatcher -> ValidDispatcher (Mount or Update)) + // ## Mount or update (ContextOnlyDispatcher -> ValidDispatcher (Mount or Update)) // // If the current dispatcher is the ContextOnlyDispatcher and the next // dispatcher is a valid dispatcher, we are entering a component render. return true; + } else if ( + currentDispatcherType & WarningDispatcherType || + nextDispatcherType & WarningDispatcherType + ) { + // ## Warning dispatcher + // + // If the current dispatcher or next dispatcher is an warning dispatcher, + // we are not entering a component render. The current warning dispatchers + // are used to warn when hooks are nested improperly and do not indicate + // entering a new component render. + return false; } else if (nextDispatcherType & RerenderDispatcherType) { // Any transition into the rerender dispatcher is a rerender is the // beginning of a component render, so we should invoke our hooks. Details @@ -302,8 +319,6 @@ function isEnteringComponentRender( // - HooksDispatcherOnMount -> HooksDispatcherOnMount // - HooksDispatcherOnMount -> HooksDispatcherOnUpdate // - HooksDispatcherOnUpdate -> HooksDispatcherOnUpdate - // - Any transition in and out of invalid dispatchers does not indicate a - // new component render. return false; } } From 028667d2cae85bb3fe481d548b1bc52fe7a9f914 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 14 Apr 2023 18:45:52 -0700 Subject: [PATCH 08/26] Attempt to fixing spuriously failing test involving cleanup effects by waiting until next frame to return from our fake act method --- packages/react/test/utils.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/react/test/utils.ts b/packages/react/test/utils.ts index d18aa3c21..e06bedbc7 100644 --- a/packages/react/test/utils.ts +++ b/packages/react/test/utils.ts @@ -41,17 +41,19 @@ export async function createRoot(container: Element): Promise { // When testing using react's production build, we can't use act (React // explicitly throws an error in this situation). So instead we'll fake act by -// just waiting 10ms for React's concurrent rerendering to flush. We'll make a -// best effort to throw a helpful error in afterEach if we detect that act() was -// called but not awaited. -const delay = (ms: number) => new Promise(r => setTimeout(r, ms)); +// waiting for a requestAnimationFrame and then 10ms for React's concurrent +// rerendering and any effects to flush. We'll make a best effort to throw a +// helpful error in afterEach if we detect that act() was called but not +// awaited. +const afterFrame = (ms: number) => + new Promise(r => requestAnimationFrame(() => setTimeout(r, ms))); let acting = 0; async function prodActShim(cb: () => void | Promise): Promise { acting++; try { await cb(); - await delay(10); + await afterFrame(10); } finally { acting--; } From 32bdd275ab0f5a23dd0f51143ce86382a7bc9c06 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 14 Apr 2023 18:51:18 -0700 Subject: [PATCH 09/26] Add state machine as a comment and a TODO to update the old comment --- packages/react/src/index.ts | 58 +++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 21ba5051d..70d330e40 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -119,6 +119,64 @@ function usePreactSignalStore(nextDispatcher: ReactDispatcher): EffectStore { return store; } +/* +PROD ReactCurrentDispatcher transition diagram (does not include dev time +warning dispatchers which are just always ignored): + +```js +// Paste this into https://stately.ai/viz to visualize the state machine. +import { createMachine } from "xstate"; + +// ENTER, EXIT, NOOP suffixes indicates whether this ReactCurrentDispatcher +// transition signals we are entering or exiting a component render, or +// if it doesn't signal a change in the component rendering lifecyle (NOOP). + +const dispatcherMachinePROD = createMachine({ + id: "ReactCurrentDispatcher_PROD", + initial: "null", + states: { + null: { + on: { + pushDispatcher: "ContextOnlyDispatcher", + }, + }, + ContextOnlyDispatcher: { + on: { + renderWithHooks_Mount_ENTER: "HooksDispatcherOnMount", + renderWithHooks_Update_ENTER: "HooksDispatcherOnUpdate", + pushDispatcher_NOOP: "ContextOnlyDispatcher", + popDispatcher_NOOP: "ContextOnlyDispatcher", + }, + }, + HooksDispatcherOnMount: { + on: { + renderWithHooksAgain_ENTER: "HooksDispatcherOnRerender", + resetHooksAfterThrow_EXIT: "ContextOnlyDispatcher", + finishRenderingHooks_EXIT: "ContextOnlyDispatcher", + }, + }, + HooksDispatcherOnUpdate: { + on: { + renderWithHooksAgain_ENTER: "HooksDispatcherOnRerender", + resetHooksAfterThrow_EXIT: "ContextOnlyDispatcher", + finishRenderingHooks_EXIT: "ContextOnlyDispatcher", + use_ResumeSuspensedMount_NOOP: "HooksDispatcherOnMount", + }, + }, + HooksDispatcherOnRerender: { + on: { + renderWithHooksAgain_ENTER: "HooksDispatcherOnRerender", + resetHooksAfterThrow_EXIT: "ContextOnlyDispatcher", + finishRenderingHooks_EXIT: "ContextOnlyDispatcher", + }, + }, + }, +}); +``` +*/ + +// TODO: Update. +// // To track when we are entering and exiting a component render (i.e. before and // after React renders a component), we track how the dispatcher changes. // Outside of a component rendering, the dispatcher is set to an instance that From b101fa2184859f4534646b0e3cf4d736494aca75 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 14 Apr 2023 18:52:20 -0700 Subject: [PATCH 10/26] Reenable static markup test --- packages/react/test/index.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/test/index.test.tsx b/packages/react/test/index.test.tsx index 56049a516..677e2a3df 100644 --- a/packages/react/test/index.test.tsx +++ b/packages/react/test/index.test.tsx @@ -280,7 +280,7 @@ describe("@preact/signals-react", () => { } }); - it.skip("should render static markup of a component", async () => { + it("should render static markup of a component", async () => { const count = signal(0); const Test = () => { From f3df91007385f632c6bcbcec5eaf7298d796db17 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Mon, 17 Apr 2023 17:16:08 -0700 Subject: [PATCH 11/26] Move current react tests to browser folder --- packages/react/test/{ => browser}/exports.test.tsx | 0 packages/react/test/{ => browser}/index.test.tsx | 2 +- packages/react/test/{ => browser}/react-router.test.tsx | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) rename packages/react/test/{ => browser}/exports.test.tsx (100%) rename packages/react/test/{ => browser}/index.test.tsx (99%) rename packages/react/test/{ => browser}/react-router.test.tsx (96%) diff --git a/packages/react/test/exports.test.tsx b/packages/react/test/browser/exports.test.tsx similarity index 100% rename from packages/react/test/exports.test.tsx rename to packages/react/test/browser/exports.test.tsx diff --git a/packages/react/test/index.test.tsx b/packages/react/test/browser/index.test.tsx similarity index 99% rename from packages/react/test/index.test.tsx rename to packages/react/test/browser/index.test.tsx index 677e2a3df..81a46a2e1 100644 --- a/packages/react/test/index.test.tsx +++ b/packages/react/test/browser/index.test.tsx @@ -28,7 +28,7 @@ import { isReact16, isProd, consoleFormat, -} from "./utils"; +} from "../utils"; describe("@preact/signals-react", () => { let scratch: HTMLDivElement; diff --git a/packages/react/test/react-router.test.tsx b/packages/react/test/browser/react-router.test.tsx similarity index 96% rename from packages/react/test/react-router.test.tsx rename to packages/react/test/browser/react-router.test.tsx index cf85eac1d..9b6fa72a2 100644 --- a/packages/react/test/react-router.test.tsx +++ b/packages/react/test/browser/react-router.test.tsx @@ -5,7 +5,7 @@ import { signal } from "@preact/signals-react"; import { createElement } from "react"; import * as ReactRouter from "react-router-dom"; -import { act, checkHangingAct, createRoot, Root } from "./utils"; +import { act, checkHangingAct, createRoot, Root } from "../utils"; const MemoryRouter = ReactRouter.MemoryRouter; const Routes = ReactRouter.Routes From 83ec84504a2c79d3f8b0163c0e6f59b59638bffc Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Tue, 18 Apr 2023 14:15:08 -0700 Subject: [PATCH 12/26] Add initial renderToStaticMarkup test with basic test infra --- package.json | 16 ++- .../test/node/renderToStaticMarkup.test.tsx | 13 +++ packages/react/test/node/setup.js | 69 ++++++++++++ pnpm-lock.yaml | 100 ++++++++++++++++++ 4 files changed, 193 insertions(+), 5 deletions(-) create mode 100644 packages/react/test/node/renderToStaticMarkup.test.tsx create mode 100644 packages/react/test/node/setup.js diff --git a/package.json b/package.json index 7b964446d..f2dcc417a 100644 --- a/package.json +++ b/package.json @@ -12,11 +12,16 @@ "postbuild:react": "cd packages/react/dist && mv -f react/src/index.d.ts signals.d.ts && rm -dr react", "postbuild": "node ./scripts/node-13-exports.js", "lint": "eslint 'packages/**/*.{ts,tsx,js,jsx}'", - "test": "cross-env COVERAGE=true karma start karma.conf.js --single-run", - "test:minify": "cross-env COVERAGE=true MINIFY=true karma start karma.conf.js --single-run", - "test:watch": "karma start karma.conf.js --no-single-run", - "test:prod": "cross-env MINIFY=true NODE_ENV=production karma start karma.conf.js --single-run", - "test:prod:watch": "cross-env NODE_ENV=production karma start karma.conf.js --no-single-run", + "test": "pnpm test:karma && pnpm test:mocha", + "test:minify": "pnpm test:karma:minify", + "test:prod": "pnpm test:karma:prod", + "test:karma": "cross-env COVERAGE=true karma start karma.conf.js --single-run", + "test:karma:minify": "cross-env COVERAGE=true MINIFY=true karma start karma.conf.js --single-run", + "test:karma:watch": "karma start karma.conf.js --no-single-run", + "test:karma:prod": "cross-env MINIFY=true NODE_ENV=production karma start karma.conf.js --single-run", + "test:karma:prod:watch": "cross-env NODE_ENV=production karma start karma.conf.js --no-single-run", + "test:mocha": "cross-env COVERAGE=true mocha --require packages/react/test/node/setup.js --recursive packages/react/test/node/**.test.tsx", + "test:mocha:minify": "cross-env COVERAGE=true MINIFY=true mocha --recursive packages/react/test/node/**.test.js", "docs:start": "cd docs && pnpm start", "docs:build": "cd docs && pnpm build", "docs:preview": "cd docs && pnpm preview", @@ -34,6 +39,7 @@ "@babel/preset-env": "^7.19.1", "@babel/preset-react": "^7.18.6", "@babel/preset-typescript": "^7.18.6", + "@babel/register": "^7.21.0", "@changesets/changelog-github": "^0.4.6", "@changesets/cli": "^2.24.2", "@types/chai": "^4.3.3", diff --git a/packages/react/test/node/renderToStaticMarkup.test.tsx b/packages/react/test/node/renderToStaticMarkup.test.tsx new file mode 100644 index 000000000..6e6e97978 --- /dev/null +++ b/packages/react/test/node/renderToStaticMarkup.test.tsx @@ -0,0 +1,13 @@ +import { expect } from "chai"; +import { createElement } from "react"; +import { renderToStaticMarkup } from "react-dom/server"; +import { signal } from "@preact/signals-react"; + +console.log(signal); + +describe("renderToStaticMarkup", () => { + it("should render a simple component", () => { + const element =
Hello World
; + expect(renderToStaticMarkup(element)).to.equal("
Hello World
"); + }); +}); diff --git a/packages/react/test/node/setup.js b/packages/react/test/node/setup.js new file mode 100644 index 000000000..2e0dd37d8 --- /dev/null +++ b/packages/react/test/node/setup.js @@ -0,0 +1,69 @@ +// import register from "@babel/register"; +const register = require("@babel/register").default; + +const coverage = String(process.env.COVERAGE) === "true"; +const minify = String(process.env.MINIFY) === "true"; + +const rename = {}; +const mangle = require("../../../../mangle.json"); +for (let prop in mangle.props.props) { + let name = prop; + if (name[0] === "$") { + name = name.slice(1); + } + rename[name] = mangle.props.props[prop]; +} + +// @babel/register doesn't hook into the experimental NodeJS ESM loader API so +// we need all test files to run as CommonJS modules in Node +const env = [ + "@babel/preset-env", + { + targets: { + node: "current", + }, + loose: true, + modules: "commonjs", + }, +]; + +const jsx = [ + "@babel/preset-react", + { + runtime: "classic", + pragma: "createElement", + pragmaFrag: "Fragment", + }, +]; + +const ts = [ + "@babel/preset-typescript", + { + jsxPragma: "createElement", + jsxPragmaFrag: "Fragment", + }, +]; + +const renamePlugin = [ + "babel-plugin-transform-rename-properties", + { + rename, + }, +]; + +register({ + extensions: [".js", ".mjs", ".ts", ".tsx", ".mts", ".mtsx"], + cache: true, + + sourceMaps: "inline", + presets: [ts, jsx, env], + plugins: [ + coverage && [ + "istanbul", + { + include: minify ? "**/dist/**/*.js" : "**/src/**/*.{ts,js}", + }, + ], + minify && renamePlugin, + ].filter(Boolean), +}); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 6b7779b98..4026a6150 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -17,6 +17,7 @@ importers: '@babel/preset-env': ^7.19.1 '@babel/preset-react': ^7.18.6 '@babel/preset-typescript': ^7.18.6 + '@babel/register': ^7.21.0 '@changesets/changelog-github': ^0.4.6 '@changesets/cli': ^2.24.2 '@types/chai': ^4.3.3 @@ -58,6 +59,7 @@ importers: '@babel/preset-env': 7.19.1_@babel+core@7.19.1 '@babel/preset-react': 7.18.6_@babel+core@7.19.1 '@babel/preset-typescript': 7.18.6_@babel+core@7.19.1 + '@babel/register': 7.21.0_@babel+core@7.19.1 '@changesets/changelog-github': 0.4.6 '@changesets/cli': 2.24.2 '@types/chai': 4.3.3 @@ -1579,6 +1581,20 @@ packages: - supports-color dev: true + /@babel/register/7.21.0_@babel+core@7.19.1: + resolution: {integrity: sha512-9nKsPmYDi5DidAqJaQooxIhsLJiNMkGr8ypQ8Uic7cIox7UCDsM7HuUGxdGT7mSDTYbqzIdsOWzfBton/YJrMw==} + engines: {node: '>=6.9.0'} + peerDependencies: + '@babel/core': ^7.0.0-0 + dependencies: + '@babel/core': 7.19.1 + clone-deep: 4.0.1 + find-cache-dir: 2.1.0 + make-dir: 2.1.0 + pirates: 4.0.5 + source-map-support: 0.5.21 + dev: true + /@babel/runtime/7.18.9: resolution: {integrity: sha512-lkqXDcvlFT5rvEjiu6+QYO+1GXrEHRo2LOtS7E4GtX5ESIZOgepqsZBVIj6Pv+a6zqsya9VCgiK1KAK4BvJDAw==} engines: {node: '>=6.9.0'} @@ -2961,6 +2977,15 @@ packages: wrap-ansi: 7.0.0 dev: true + /clone-deep/4.0.1: + resolution: {integrity: sha512-neHB9xuzh/wk0dIHweyAXv2aPGZIVk3pLMe+/RNzINf17fe0OG96QroktYAUm7SM1PBnzTabaLboqqxDyMU+SQ==} + engines: {node: '>=6'} + dependencies: + is-plain-object: 2.0.4 + kind-of: 6.0.3 + shallow-clone: 3.0.1 + dev: true + /clone/1.0.4: resolution: {integrity: sha512-JQHZ2QMW6l3aH/j6xCqQThY/9OH4D/9ls34cgkUBiEeocRTU04tHfKPBsUK1PqZCUQM7GiA0IIXJSuXHI64Kbg==} engines: {node: '>=0.8'} @@ -4051,6 +4076,15 @@ packages: - supports-color dev: true + /find-cache-dir/2.1.0: + resolution: {integrity: sha512-Tq6PixE0w/VMFfCgbONnkiQIVol/JJL7nRMi20fqzA4NRs9AfeqMGeRdPi3wIhYkxjeBaWh2rxwapn5Tu3IqOQ==} + engines: {node: '>=6'} + dependencies: + commondir: 1.0.1 + make-dir: 2.1.0 + pkg-dir: 3.0.0 + dev: true + /find-cache-dir/3.3.2: resolution: {integrity: sha512-wXZV5emFEjrridIgED11OoUKLxiYjAcqot/NJdAkOhlJ+vGzwhOAfcG5OX1jP+S0PcjEn8bdMJv+g2jwQ3Onig==} engines: {node: '>=8'} @@ -4060,6 +4094,13 @@ packages: pkg-dir: 4.2.0 dev: true + /find-up/3.0.0: + resolution: {integrity: sha512-1yD6RmLI1XBfxugvORwlck6f75tYL+iR0jqwsOrOxMZyGYqUuDhJ0l4AXdO1iX/FTs9cBAMEk1gWSEx1kSbylg==} + engines: {node: '>=6'} + dependencies: + locate-path: 3.0.0 + dev: true + /find-up/4.1.0: resolution: {integrity: sha512-PpOwAdQ/YlXQ2vj8a3h8IipDuYRi3wceVQQGYWxNINccq40Anw7BlsEXCMbt1Zt+OLA6Fq9suIpIWD0OsnISlw==} engines: {node: '>=8'} @@ -4598,6 +4639,13 @@ packages: engines: {node: '>=8'} dev: true + /is-plain-object/2.0.4: + resolution: {integrity: sha512-h5PpgXkWitc38BBMYawTYMWJHFZJVnBquFE57xFpjB8pJFiF6gZ+bU+WyI/yqXiFR5mdLsgYNaPe8uao6Uv9Og==} + engines: {node: '>=0.10.0'} + dependencies: + isobject: 3.0.1 + dev: true + /is-reference/1.2.1: resolution: {integrity: sha512-U82MsXXiFIrjCK4otLT+o2NA2Cd2g5MLoOVXUZjIOhLurrRxpEXzI8O0KZHr3IjLvlAH1kTPYSuqer5T9ZVBKQ==} dependencies: @@ -4680,6 +4728,11 @@ packages: resolution: {integrity: sha512-RHxMLp9lnKHGHRng9QFhRCMbYAcVpn69smSGcq3f36xjgVVWThj4qqLbTLlq7Ssj8B+fIQ1EuCEGI2lKsyQeIw==} dev: true + /isobject/3.0.1: + resolution: {integrity: sha512-WhB9zCku7EGTj/HQQRz5aUQEUeoQZH2bWcltRErOpymJ4boYE6wL9Tbr23krRPSZ+C5zqNSrSw+Cc7sZZ4b7vg==} + engines: {node: '>=0.10.0'} + dev: true + /istanbul-lib-coverage/3.2.0: resolution: {integrity: sha512-eOeJ5BHCmHYvQK7xt9GkdHuzuCGS1Y6g9Gvnx3Ym33fz/HpLRYxiS0wHNr+m/MBC8B647Xt608vCDEvhl9c6Mw==} engines: {node: '>=8'} @@ -5008,6 +5061,14 @@ packages: engines: {node: '>= 12.13.0'} dev: true + /locate-path/3.0.0: + resolution: {integrity: sha512-7AO748wWnIhNqAuaty2ZWHkQHRSNfPVIsPIfwEOWO22AmaoVrWavlOcMR5nzTLNYvp36X220/maaRsrec1G65A==} + engines: {node: '>=6'} + dependencies: + p-locate: 3.0.0 + path-exists: 3.0.0 + dev: true + /locate-path/5.0.0: resolution: {integrity: sha512-t7hw9pI+WvuwNJXwk5zVHpyhIqzg2qTlklJOf0mVxGSbe3Fp2VieZcduNYjaLDoy6p9uGpQEGWG87WpMKlNq8g==} engines: {node: '>=8'} @@ -5124,6 +5185,14 @@ packages: sourcemap-codec: 1.4.8 dev: true + /make-dir/2.1.0: + resolution: {integrity: sha512-LS9X+dc8KLxXCb8dni79fLIIUA5VyZoyjSMCwTluaXA0o27cCK0bhXkpgw+sTXVpPy/lSO57ilRixqk0vDmtRA==} + engines: {node: '>=6'} + dependencies: + pify: 4.0.1 + semver: 5.7.1 + dev: true + /make-dir/3.1.0: resolution: {integrity: sha512-g3FeP20LNwhALb/6Cz6Dd4F2ngze0jz7tbzrD2wAV+o9FeNHe4rL+yK2md0J/fiSf1sa1ADhXqi5+oVwOM/eGw==} engines: {node: '>=8'} @@ -5574,6 +5643,13 @@ packages: yocto-queue: 0.1.0 dev: true + /p-locate/3.0.0: + resolution: {integrity: sha512-x+12w/To+4GFfgJhBEpiDcLozRJGegY+Ei7/z0tSLkMmxGZNybVMSfWj9aJn8Z5Fc7dBUNJOOVgPv2H7IwulSQ==} + engines: {node: '>=6'} + dependencies: + p-limit: 2.3.0 + dev: true + /p-locate/4.1.0: resolution: {integrity: sha512-R79ZZ/0wAxKGu3oYMlz8jy/kbhsNrS7SKZ7PxEHBgJ5+F2mtFW2fK2cOtBh1cHYkQsbzFV7I+EoRKe6Yt0oK7A==} engines: {node: '>=8'} @@ -5642,6 +5718,11 @@ packages: engines: {node: '>= 0.8'} dev: true + /path-exists/3.0.0: + resolution: {integrity: sha512-bpC7GYwiDYQ4wYLe+FA8lhRjhQCMcQGuSgGGqDkg/QerRWw9CmGRT0iSOVRSZJ29NMLZgIzqaljJ63oaL4NIJQ==} + engines: {node: '>=4'} + dev: true + /path-exists/4.0.0: resolution: {integrity: sha512-ak9Qy5Q7jYb2Wwcey5Fpvg2KoAc/ZIhLSLOSBmRmygPsGwkVVt0fZa0qrtMz+m6tJTAHfZQ8FnmB4MG4LWy7/w==} engines: {node: '>=8'} @@ -5706,6 +5787,18 @@ packages: engines: {node: '>=10'} dev: true + /pirates/4.0.5: + resolution: {integrity: sha512-8V9+HQPupnaXMA23c5hvl69zXvTwTzyAYasnkb0Tts4XvO4CliqONMOnvlq26rkhLC3nWDFBJf73LU1e1VZLaQ==} + engines: {node: '>= 6'} + dev: true + + /pkg-dir/3.0.0: + resolution: {integrity: sha512-/E57AYkoeQ25qkxMj5PBOVgF8Kiu/h7cYS30Z5+R7WaiCCBfLq58ZI/dSeaEKb9WVJV5n/03QwrN3IeWIFllvw==} + engines: {node: '>=6'} + dependencies: + find-up: 3.0.0 + dev: true + /pkg-dir/4.2.0: resolution: {integrity: sha512-HRDzbaKjC+AOWVXxAU/x54COGeIv9eb+6CkDSQoNTt4XyWoIJvuPsXizxu/Fr23EiekbtZwmh1IcIG/l/a10GQ==} engines: {node: '>=8'} @@ -6590,6 +6683,13 @@ packages: resolution: {integrity: sha512-E5LDX7Wrp85Kil5bhZv46j8jOeboKq5JMmYM3gVGdGH8xFpPWXUMsNrlODCrkoxMEeNi/XZIwuRvY4XNwYMJpw==} dev: true + /shallow-clone/3.0.1: + resolution: {integrity: sha512-/6KqX+GVUdqPuPPd2LxDDxzX6CAbjJehAAOKlNpqqUpAqPM6HeL8f+o3a+JsyGjn2lv0WY8UsTgUJjU9Ok55NA==} + engines: {node: '>=8'} + dependencies: + kind-of: 6.0.3 + dev: true + /shebang-command/1.2.0: resolution: {integrity: sha512-EV3L1+UQWGor21OmnvojK36mhg+TyIKDh3iFBKBohr5xeXIhNBcx8oWdgkTEEQ+BEFFYdLRuqMfd5L84N1V5Vg==} engines: {node: '>=0.10.0'} From d3ea8e5d2865b55436c8123dd967e6b061f933a6 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Tue, 18 Apr 2023 14:26:42 -0700 Subject: [PATCH 13/26] Simplify NodeJS babel config since Node tests always run against build output --- package.json | 1 - packages/react/test/node/setup.js | 23 +++-------------------- 2 files changed, 3 insertions(+), 21 deletions(-) diff --git a/package.json b/package.json index f2dcc417a..36198258a 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,6 @@ "test:karma:prod": "cross-env MINIFY=true NODE_ENV=production karma start karma.conf.js --single-run", "test:karma:prod:watch": "cross-env NODE_ENV=production karma start karma.conf.js --no-single-run", "test:mocha": "cross-env COVERAGE=true mocha --require packages/react/test/node/setup.js --recursive packages/react/test/node/**.test.tsx", - "test:mocha:minify": "cross-env COVERAGE=true MINIFY=true mocha --recursive packages/react/test/node/**.test.js", "docs:start": "cd docs && pnpm start", "docs:build": "cd docs && pnpm build", "docs:preview": "cd docs && pnpm preview", diff --git a/packages/react/test/node/setup.js b/packages/react/test/node/setup.js index 2e0dd37d8..210f6fac3 100644 --- a/packages/react/test/node/setup.js +++ b/packages/react/test/node/setup.js @@ -2,17 +2,6 @@ const register = require("@babel/register").default; const coverage = String(process.env.COVERAGE) === "true"; -const minify = String(process.env.MINIFY) === "true"; - -const rename = {}; -const mangle = require("../../../../mangle.json"); -for (let prop in mangle.props.props) { - let name = prop; - if (name[0] === "$") { - name = name.slice(1); - } - rename[name] = mangle.props.props[prop]; -} // @babel/register doesn't hook into the experimental NodeJS ESM loader API so // we need all test files to run as CommonJS modules in Node @@ -44,13 +33,6 @@ const ts = [ }, ]; -const renamePlugin = [ - "babel-plugin-transform-rename-properties", - { - rename, - }, -]; - register({ extensions: [".js", ".mjs", ".ts", ".tsx", ".mts", ".mtsx"], cache: true, @@ -61,9 +43,10 @@ register({ coverage && [ "istanbul", { - include: minify ? "**/dist/**/*.js" : "**/src/**/*.{ts,js}", + // TODO: Currently NodeJS tests always run against dist files. Should we + // change this? + // include: minify ? "**/dist/**/*.js" : "**/src/**/*.{js,jsx,ts,tsx}", }, ], - minify && renamePlugin, ].filter(Boolean), }); From 83e5a06ff63a44c7824b78acc840761d7a409edb Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Tue, 18 Apr 2023 17:03:50 -0700 Subject: [PATCH 14/26] Add some more comments and investigations to look at --- packages/react/src/index.ts | 40 +++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 70d330e40..857174f83 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -252,6 +252,7 @@ Object.defineProperty(ReactInternals.ReactCurrentDispatcher, "current", { }, }); +type DispatcherType = number; const ContextOnlyDispatcherType = 1 << 0; const WarningDispatcherType = 1 << 1; const MountDispatcherType = 1 << 2; @@ -259,12 +260,6 @@ const UpdateDispatcherType = 1 << 3; const RerenderDispatcherType = 1 << 4; const ValidDispatcherType = MountDispatcherType | UpdateDispatcherType | RerenderDispatcherType; -type DispatcherType = - | typeof ContextOnlyDispatcherType - | typeof WarningDispatcherType - | typeof MountDispatcherType - | typeof UpdateDispatcherType - | typeof ValidDispatcherType; // We inject a useSyncExternalStore into every function component via // CurrentDispatcher. This prevents injecting into anything other than a @@ -277,6 +272,15 @@ function getDispatcherType(dispatcher: ReactDispatcher | null): DispatcherType { const cached = dispatcherTypeCache.get(dispatcher); if (cached !== undefined) return cached; + // TODO: Explore using something like `useReducer === useCallback` to detect + // ContextOnlyDispatcher. + + // TODO: Consider adding tests for the situation where the dispatcher starts + // out as HooksDispatcherOnMountInDEV and then changes to + // HooksDispatcherOnMountWithHookTypesInDEV. I believe this is possible if a + // component only uses state-less hooks such as useContext which give a fiber + // hook types in DEV but do not give it a memoized state. + // The ContextOnlyDispatcher sets all the hook implementations to a function // that takes no arguments and throws and error. Check the number of arguments // for this dispatcher's useCallback implementation to determine if it is a @@ -289,23 +293,33 @@ function getDispatcherType(dispatcher: ReactDispatcher | null): DispatcherType { if (dispatcher.useCallback.length < 2) { type = ContextOnlyDispatcherType; } else if (/Invalid/.test(useCallbackImpl)) { + // We first check for warning dispatchers because they would also pass some + // of the checks below. type = WarningDispatcherType; } else if ( + // The development mount dispatcher invokes a function called + // `mountCallback` whereas the development update/re-render dispatcher + // invokes a function called `updateCallback`. Use that to determine if we + // are in a mount or update-like dispatcher in development. The production + // mount dispatcher defines an array of the form [callback, deps] whereas + // update/re-render dispatchers read the array using array indices (e.g. + // `[0]` and `[1]`). Use those differences to determine if we are in a mount + // or update-like dispatcher in production. /updateCallback/.test(useCallbackImpl) || (/\[0\]/.test(useCallbackImpl) && /\[1\]/.test(useCallbackImpl)) ) { - // The mount and update dispatchers have a different implementation for the - // useCallback hook. The mount dispatcher stores the arguments to - // useCallback as an array on the state of the Fiber. The update dispatcher - // reads the values of the array using array indices (e.g. `[0]` and `[1]`). - // So we'll check for those indices to determine if we are in a mount or - // update-like dispatcher. - // The update and rerender dispatchers have different implementations for // useReducer. We'll check it's implementation to determine if this is the // rerender or update dispatcher. let useReducerImpl = dispatcher.useReducer.toString(); if ( + // The development rerender dispatcher invokes a function called + // `rerenderReducer` whereas the update dispatcher invokes a function + // called `updateReducer`. The production rerender dispatcher returns an + // array of the form `[state, dispatch]` whereas the update dispatcher + // returns an array of `[fiber.memoizedState, dispatch]` so we check the + // return statement in the implementation of useReducer to differentiate + // between the two. /rerenderReducer/.test(useReducerImpl) || /return\s*\[[a-z]+,/.test(useReducerImpl) ) { From 6d0651f451b1e05fcfd6443c080b2ac416ae25e7 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Wed, 19 Apr 2023 13:10:34 -0700 Subject: [PATCH 15/26] Properly detect server render dispatchers and do nothing --- packages/react/src/index.ts | 26 ++++++++++++------- .../test/node/renderToStaticMarkup.test.tsx | 15 +++++++++-- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 857174f83..4404ab0b2 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -6,8 +6,6 @@ import { // eslint-disable-next-line @typescript-eslint/no-unused-vars __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED as ReactInternals, type ReactElement, - type useCallback, - type useReducer, } from "react"; import React from "react"; import jsxRuntime from "react/jsx-runtime"; @@ -29,10 +27,12 @@ const Empty = [] as const; const ReactElemType = Symbol.for("react.element"); // https://github.com/facebook/react/blob/346c7d4c43a0717302d446da9e7423a8e28d8996/packages/shared/ReactSymbols.js#L15 interface ReactDispatcher { - useRef: typeof useRef; - useCallback: typeof useCallback; - useReducer: typeof useReducer; - useSyncExternalStore: typeof useSyncExternalStore; + useRef: typeof React.useRef; + useCallback: typeof React.useCallback; + useReducer: typeof React.useReducer; + useSyncExternalStore: typeof React.useSyncExternalStore; + useEffect: typeof React.useEffect; + useImperativeHandle: typeof React.useImperativeHandle; } let finishUpdate: (() => void) | undefined; @@ -258,7 +258,8 @@ const WarningDispatcherType = 1 << 1; const MountDispatcherType = 1 << 2; const UpdateDispatcherType = 1 << 3; const RerenderDispatcherType = 1 << 4; -const ValidDispatcherType = +const ServerDispatcherType = 1 << 5; +const BrowserClientDispatcherType = MountDispatcherType | UpdateDispatcherType | RerenderDispatcherType; // We inject a useSyncExternalStore into every function component via @@ -290,8 +291,13 @@ function getDispatcherType(dispatcher: ReactDispatcher | null): DispatcherType { // it is. let type: DispatcherType; const useCallbackImpl = dispatcher.useCallback.toString(); - if (dispatcher.useCallback.length < 2) { + if (dispatcher.useReducer === dispatcher.useEffect) { type = ContextOnlyDispatcherType; + + // @ts-expect-error Only in server renderers is this true which the types + // will never represent. + } else if (dispatcher.useEffect === dispatcher.useImperativeHandle) { + type = ServerDispatcherType; } else if (/Invalid/.test(useCallbackImpl)) { // We first check for warning dispatchers because they would also pass some // of the checks below. @@ -341,7 +347,7 @@ function isEnteringComponentRender( ): boolean { if ( currentDispatcherType & ContextOnlyDispatcherType && - nextDispatcherType & ValidDispatcherType + nextDispatcherType & BrowserClientDispatcherType ) { // ## Mount or update (ContextOnlyDispatcher -> ValidDispatcher (Mount or Update)) // @@ -404,7 +410,7 @@ function isExitingComponentRender( nextDispatcherType: DispatcherType ): boolean { return Boolean( - currentDispatcherType & ValidDispatcherType && + currentDispatcherType & BrowserClientDispatcherType && nextDispatcherType & ContextOnlyDispatcherType ); } diff --git a/packages/react/test/node/renderToStaticMarkup.test.tsx b/packages/react/test/node/renderToStaticMarkup.test.tsx index 6e6e97978..1f14fcb85 100644 --- a/packages/react/test/node/renderToStaticMarkup.test.tsx +++ b/packages/react/test/node/renderToStaticMarkup.test.tsx @@ -7,7 +7,18 @@ console.log(signal); describe("renderToStaticMarkup", () => { it("should render a simple component", () => { - const element =
Hello World
; - expect(renderToStaticMarkup(element)).to.equal("
Hello World
"); + function App() { + return
Hello World
; + } + expect(renderToStaticMarkup()).to.equal("
Hello World
"); + }); + + it("should render a component with a signal as text", () => { + const name = signal("World"); + function App() { + return
Hello {name}
; + } + + expect(renderToStaticMarkup()).to.equal("
Hello World
"); }); }); From 693b84df27880a058e52d69c4c8f6597cbe9c6fe Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Wed, 19 Apr 2023 15:02:35 -0700 Subject: [PATCH 16/26] Add basic SSR tests and share with browser tests --- packages/react/test/browser/mounts.test.tsx | 20 +++ .../react/test/browser/react-router.test.tsx | 2 +- .../{index.test.tsx => updates.test.tsx} | 7 +- .../test/node/renderToStaticMarkup.test.tsx | 26 +-- packages/react/test/shared/mounting.tsx | 164 ++++++++++++++++++ packages/react/test/{ => shared}/utils.ts | 16 ++ 6 files changed, 218 insertions(+), 17 deletions(-) create mode 100644 packages/react/test/browser/mounts.test.tsx rename packages/react/test/browser/{index.test.tsx => updates.test.tsx} (98%) create mode 100644 packages/react/test/shared/mounting.tsx rename packages/react/test/{ => shared}/utils.ts (85%) diff --git a/packages/react/test/browser/mounts.test.tsx b/packages/react/test/browser/mounts.test.tsx new file mode 100644 index 000000000..0f198644c --- /dev/null +++ b/packages/react/test/browser/mounts.test.tsx @@ -0,0 +1,20 @@ +import { mountSignalsTests } from "../shared/mounting"; +import { Root, createRoot, act } from "../shared/utils"; + +describe("@preact/signals-react mounting", () => { + let scratch: HTMLDivElement; + let root: Root; + + async function render(element: JSX.Element | null): Promise { + await act(() => root.render(element)); + return scratch.innerHTML; + } + + beforeEach(async () => { + scratch = document.createElement("div"); + document.body.appendChild(scratch); + root = await createRoot(scratch); + }); + + mountSignalsTests(render); +}); diff --git a/packages/react/test/browser/react-router.test.tsx b/packages/react/test/browser/react-router.test.tsx index 9b6fa72a2..eaa1dd8dc 100644 --- a/packages/react/test/browser/react-router.test.tsx +++ b/packages/react/test/browser/react-router.test.tsx @@ -5,7 +5,7 @@ import { signal } from "@preact/signals-react"; import { createElement } from "react"; import * as ReactRouter from "react-router-dom"; -import { act, checkHangingAct, createRoot, Root } from "../utils"; +import { act, checkHangingAct, createRoot, Root } from "../shared/utils"; const MemoryRouter = ReactRouter.MemoryRouter; const Routes = ReactRouter.Routes diff --git a/packages/react/test/browser/index.test.tsx b/packages/react/test/browser/updates.test.tsx similarity index 98% rename from packages/react/test/browser/index.test.tsx rename to packages/react/test/browser/updates.test.tsx index 81a46a2e1..7f2ddb921 100644 --- a/packages/react/test/browser/index.test.tsx +++ b/packages/react/test/browser/updates.test.tsx @@ -28,12 +28,13 @@ import { isReact16, isProd, consoleFormat, -} from "../utils"; + getConsoleErrorSpy, +} from "../shared/utils"; -describe("@preact/signals-react", () => { +describe("@preact/signals-react updating", () => { let scratch: HTMLDivElement; let root: Root; - let errorSpy = sinon.spy(console, "error"); + const errorSpy = getConsoleErrorSpy(); async function render(element: Parameters[0]) { await act(() => root.render(element)); diff --git a/packages/react/test/node/renderToStaticMarkup.test.tsx b/packages/react/test/node/renderToStaticMarkup.test.tsx index 1f14fcb85..572d4026b 100644 --- a/packages/react/test/node/renderToStaticMarkup.test.tsx +++ b/packages/react/test/node/renderToStaticMarkup.test.tsx @@ -1,24 +1,24 @@ +import { signal, useSignalEffect } from "@preact/signals-react"; import { expect } from "chai"; import { createElement } from "react"; import { renderToStaticMarkup } from "react-dom/server"; -import { signal } from "@preact/signals-react"; - -console.log(signal); +import sinon from "sinon"; +import { mountSignalsTests } from "../shared/mounting"; describe("renderToStaticMarkup", () => { - it("should render a simple component", () => { - function App() { - return
Hello World
; - } - expect(renderToStaticMarkup()).to.equal("
Hello World
"); - }); + mountSignalsTests(renderToStaticMarkup); + + it("should not invoke useSignalEffect", async () => { + const spy = sinon.spy(); + const sig = signal("foo"); - it("should render a component with a signal as text", () => { - const name = signal("World"); function App() { - return
Hello {name}
; + useSignalEffect(() => spy(sig.value)); + return

{sig.value}

; } - expect(renderToStaticMarkup()).to.equal("
Hello World
"); + const html = await renderToStaticMarkup(); + expect(html).to.equal("

foo

"); + expect(spy.called).to.be.false; }); }); diff --git a/packages/react/test/shared/mounting.tsx b/packages/react/test/shared/mounting.tsx new file mode 100644 index 000000000..29471bc06 --- /dev/null +++ b/packages/react/test/shared/mounting.tsx @@ -0,0 +1,164 @@ +// @ts-ignore-next-line +globalThis.IS_REACT_ACT_ENVIRONMENT = true; + +import { + signal, + computed, + useComputed, + useSignal, +} from "@preact/signals-react"; +import { expect } from "chai"; +import { createElement, useReducer, StrictMode, useState } from "react"; + +import { consoleFormat, getConsoleErrorSpy } from "./utils"; + +export function mountSignalsTests( + render: (element: JSX.Element) => string | Promise +) { + let errorSpy = getConsoleErrorSpy(); + + beforeEach(async () => { + errorSpy.resetHistory(); + }); + + afterEach(async () => { + if (errorSpy.called) { + let message: string; + if (errorSpy.firstCall.args[0].toString().includes("%s")) { + message = consoleFormat(...errorSpy.firstCall.args); + } else { + message = errorSpy.firstCall.args.join(" "); + } + + // Ignore errors for timeouts of tests that often happen while debugging + if (!message.includes("async tests and hooks,")) { + expect.fail( + `Console.error was unexpectedly called with this message: \n${message}` + ); + } + } + }); + + describe("mount text bindings", () => { + it("should render text without signals", async () => { + const html = await render(test); + expect(html).to.equal("test"); + }); + + it("should render Signals as Text", async () => { + const sig = signal("test"); + const html = await render({sig}); + expect(html).to.equal("test"); + }); + + it("should render computed as Text", async () => { + const sig = signal("test"); + const comp = computed(() => `${sig} ${sig}`); + const html = await render({comp}); + expect(html).to.equal("test test"); + }); + }); + + describe("mount component bindings", () => { + it("should mount component with signals as text", async () => { + const sig = signal("foo"); + + function App() { + const value = sig.value; + return

{value}

; + } + + const html = await render(); + expect(html).to.equal("

foo

"); + }); + + it("should activate signal accessed in render", async () => { + const sig = signal(null); + + function App() { + const arr = useComputed(() => { + // trigger read + sig.value; + + return []; + }); + + const str = arr.value.join(", "); + return

{str}

; + } + + try { + await render(); + } catch (e: any) { + expect.fail(e.stack); + } + }); + + it("should properly mount in strict mode", async () => { + const sig = signal(-1); + + const Test = () =>

{sig.value}

; + const App = () => ( + + + + ); + + const html = await render(); + expect(html).to.equal("

-1

"); + }); + + it("should correctly mount components that have useReducer()", async () => { + const count = signal(0); + + const Test = () => { + const [state] = useReducer((state: number, action: number) => { + return state + action; + }, -2); + + const doubled = count.value * 2; + + return ( + 
+						{state}
+						{doubled}
+
+ ); + }; + + const html = await render(); + expect(html).to.equal("
-20
"); + }); + + it("should not fail when a component calls setState while rendering", async () => { + function App() { + const [state, setState] = useState(0); + if (state == 0) { + setState(1); + } + + return
{state}
; + } + + const html = await render(); + expect(html).to.equal("
1
"); + }); + }); + + describe("useSignal()", () => { + it("should create a signal from a primitive value", async () => { + function App() { + const count = useSignal(1); + return ( +
+ {count} + +
+ ); + } + + const html = await render(); + expect(html).to.equal("
1
"); + }); + }); +} diff --git a/packages/react/test/utils.ts b/packages/react/test/shared/utils.ts similarity index 85% rename from packages/react/test/utils.ts rename to packages/react/test/shared/utils.ts index e06bedbc7..08dcbc739 100644 --- a/packages/react/test/utils.ts +++ b/packages/react/test/shared/utils.ts @@ -1,4 +1,5 @@ import React from "react"; +import sinon from "sinon"; import { act as realAct } from "react-dom/test-utils"; export interface Root { @@ -81,3 +82,18 @@ export function consoleFormat(str: string, ...values: unknown[]): string { let idx = 0; return str.replace(/%s/g, () => String(values[idx++])); } + +declare global { + let errorSpy: sinon.SinonSpy | undefined; +} + +// Only one spy can be active on an object at a time and since all tests share +// the same console object we need to make sure we're only spying on it once. +// We'll use this method to share the spy across all tests. +export function getConsoleErrorSpy(): sinon.SinonSpy { + if (typeof errorSpy === "undefined") { + (globalThis as any).errorSpy = sinon.spy(console, "error"); + } + + return errorSpy!; +} From 30b54ee42be6e98005f98609760bb6899c138479 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Wed, 19 Apr 2023 15:49:47 -0700 Subject: [PATCH 17/26] Improve rerender dispatcher detection --- package.json | 3 ++- packages/react/src/index.ts | 2 +- packages/react/test/browser/mounts.test.tsx | 14 +++++++++++- packages/react/test/browser/updates.test.tsx | 23 ++++---------------- packages/react/test/shared/mounting.tsx | 22 +++---------------- packages/react/test/shared/utils.ts | 19 ++++++++++++++++ 6 files changed, 42 insertions(+), 41 deletions(-) diff --git a/package.json b/package.json index 36198258a..5401d230b 100644 --- a/package.json +++ b/package.json @@ -14,13 +14,14 @@ "lint": "eslint 'packages/**/*.{ts,tsx,js,jsx}'", "test": "pnpm test:karma && pnpm test:mocha", "test:minify": "pnpm test:karma:minify", - "test:prod": "pnpm test:karma:prod", + "test:prod": "pnpm test:karma:prod && pnpm test:mocha:prod", "test:karma": "cross-env COVERAGE=true karma start karma.conf.js --single-run", "test:karma:minify": "cross-env COVERAGE=true MINIFY=true karma start karma.conf.js --single-run", "test:karma:watch": "karma start karma.conf.js --no-single-run", "test:karma:prod": "cross-env MINIFY=true NODE_ENV=production karma start karma.conf.js --single-run", "test:karma:prod:watch": "cross-env NODE_ENV=production karma start karma.conf.js --no-single-run", "test:mocha": "cross-env COVERAGE=true mocha --require packages/react/test/node/setup.js --recursive packages/react/test/node/**.test.tsx", + "test:mocha:prod": "cross-env COVERAGE=true NODE_ENV=production mocha --require packages/react/test/node/setup.js --recursive packages/react/test/node/**.test.tsx", "docs:start": "cd docs && pnpm start", "docs:build": "cd docs && pnpm build", "docs:preview": "cd docs && pnpm preview", diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 4404ab0b2..1968d6954 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -327,7 +327,7 @@ function getDispatcherType(dispatcher: ReactDispatcher | null): DispatcherType { // return statement in the implementation of useReducer to differentiate // between the two. /rerenderReducer/.test(useReducerImpl) || - /return\s*\[[a-z]+,/.test(useReducerImpl) + /return\s*\[\w+,/.test(useReducerImpl) ) { type = RerenderDispatcherType; } else { diff --git a/packages/react/test/browser/mounts.test.tsx b/packages/react/test/browser/mounts.test.tsx index 0f198644c..af132cac9 100644 --- a/packages/react/test/browser/mounts.test.tsx +++ b/packages/react/test/browser/mounts.test.tsx @@ -1,5 +1,11 @@ import { mountSignalsTests } from "../shared/mounting"; -import { Root, createRoot, act } from "../shared/utils"; +import { + Root, + createRoot, + act, + getConsoleErrorSpy, + checkConsoleErrorLogs, +} from "../shared/utils"; describe("@preact/signals-react mounting", () => { let scratch: HTMLDivElement; @@ -14,6 +20,12 @@ describe("@preact/signals-react mounting", () => { scratch = document.createElement("div"); document.body.appendChild(scratch); root = await createRoot(scratch); + getConsoleErrorSpy().resetHistory(); + }); + + afterEach(async () => { + scratch.remove(); + checkConsoleErrorLogs(); }); mountSignalsTests(render); diff --git a/packages/react/test/browser/updates.test.tsx b/packages/react/test/browser/updates.test.tsx index 7f2ddb921..c97f48996 100644 --- a/packages/react/test/browser/updates.test.tsx +++ b/packages/react/test/browser/updates.test.tsx @@ -27,14 +27,13 @@ import { checkHangingAct, isReact16, isProd, - consoleFormat, getConsoleErrorSpy, + checkConsoleErrorLogs, } from "../shared/utils"; describe("@preact/signals-react updating", () => { let scratch: HTMLDivElement; let root: Root; - const errorSpy = getConsoleErrorSpy(); async function render(element: Parameters[0]) { await act(() => root.render(element)); @@ -44,29 +43,15 @@ describe("@preact/signals-react updating", () => { scratch = document.createElement("div"); document.body.appendChild(scratch); root = await createRoot(scratch); - errorSpy.resetHistory(); + getConsoleErrorSpy().resetHistory(); }); afterEach(async () => { - checkHangingAct(); await act(() => root.unmount()); scratch.remove(); - if (errorSpy.called) { - let message: string; - if (errorSpy.firstCall.args[0].toString().includes("%s")) { - message = consoleFormat(...errorSpy.firstCall.args); - } else { - message = errorSpy.firstCall.args.join(" "); - } - - // Ignore errors for timeouts of tests that often happen while debugging - if (!message.includes("async tests and hooks,")) { - expect.fail( - `Console.error was unexpectedly called with this message: \n${message}` - ); - } - } + checkConsoleErrorLogs(); + checkHangingAct(); }); describe("Text bindings", () => { diff --git a/packages/react/test/shared/mounting.tsx b/packages/react/test/shared/mounting.tsx index 29471bc06..d34603a43 100644 --- a/packages/react/test/shared/mounting.tsx +++ b/packages/react/test/shared/mounting.tsx @@ -10,33 +10,17 @@ import { import { expect } from "chai"; import { createElement, useReducer, StrictMode, useState } from "react"; -import { consoleFormat, getConsoleErrorSpy } from "./utils"; +import { getConsoleErrorSpy, checkConsoleErrorLogs } from "./utils"; export function mountSignalsTests( render: (element: JSX.Element) => string | Promise ) { - let errorSpy = getConsoleErrorSpy(); - beforeEach(async () => { - errorSpy.resetHistory(); + getConsoleErrorSpy().resetHistory(); }); afterEach(async () => { - if (errorSpy.called) { - let message: string; - if (errorSpy.firstCall.args[0].toString().includes("%s")) { - message = consoleFormat(...errorSpy.firstCall.args); - } else { - message = errorSpy.firstCall.args.join(" "); - } - - // Ignore errors for timeouts of tests that often happen while debugging - if (!message.includes("async tests and hooks,")) { - expect.fail( - `Console.error was unexpectedly called with this message: \n${message}` - ); - } - } + checkConsoleErrorLogs(); }); describe("mount text bindings", () => { diff --git a/packages/react/test/shared/utils.ts b/packages/react/test/shared/utils.ts index 08dcbc739..8563c80d1 100644 --- a/packages/react/test/shared/utils.ts +++ b/packages/react/test/shared/utils.ts @@ -97,3 +97,22 @@ export function getConsoleErrorSpy(): sinon.SinonSpy { return errorSpy!; } + +export function checkConsoleErrorLogs(): void { + const errorSpy = getConsoleErrorSpy(); + if (errorSpy.called) { + let message: string; + if (errorSpy.firstCall.args[0].toString().includes("%s")) { + message = consoleFormat(...errorSpy.firstCall.args); + } else { + message = errorSpy.firstCall.args.join(" "); + } + + // Ignore errors for timeouts of tests that often happen while debugging + if (!message.includes("async tests and hooks,")) { + expect.fail( + `Console.error was unexpectedly called with this message: \n${message}` + ); + } + } +} From 9e6a4e08feb0843a801a5cd2a1961019db31c181 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Wed, 19 Apr 2023 16:26:10 -0700 Subject: [PATCH 18/26] Ignore other useless console errors --- packages/react/test/shared/utils.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/react/test/shared/utils.ts b/packages/react/test/shared/utils.ts index 8563c80d1..7a6509a08 100644 --- a/packages/react/test/shared/utils.ts +++ b/packages/react/test/shared/utils.ts @@ -98,6 +98,13 @@ export function getConsoleErrorSpy(): sinon.SinonSpy { return errorSpy!; } +const messagesToIgnore = [ + // Ignore errors for timeouts of tests that often happen while debugging + /async tests and hooks,/, + // Ignore React 16 warnings about awaiting `act` calls (warning removed in React 18) + /Do not await the result of calling act/, +]; + export function checkConsoleErrorLogs(): void { const errorSpy = getConsoleErrorSpy(); if (errorSpy.called) { @@ -108,8 +115,7 @@ export function checkConsoleErrorLogs(): void { message = errorSpy.firstCall.args.join(" "); } - // Ignore errors for timeouts of tests that often happen while debugging - if (!message.includes("async tests and hooks,")) { + if (messagesToIgnore.every(re => re.test(message) === false)) { expect.fail( `Console.error was unexpectedly called with this message: \n${message}` ); From 8a8c0d3d3b0218388c577486545ad2d7ac79f11a Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Wed, 19 Apr 2023 19:36:11 -0700 Subject: [PATCH 19/26] Add more tests for rerendering --- packages/react/test/browser/updates.test.tsx | 36 ++++++++++++++++++-- packages/react/test/shared/mounting.tsx | 16 ++++++++- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/packages/react/test/browser/updates.test.tsx b/packages/react/test/browser/updates.test.tsx index c97f48996..c50036826 100644 --- a/packages/react/test/browser/updates.test.tsx +++ b/packages/react/test/browser/updates.test.tsx @@ -338,17 +338,47 @@ describe("@preact/signals-react updating", () => { }); it("should not fail when a component calls setState while rendering", async () => { + let increment: () => void; + function App() { + const [state, setState] = useState(0); + increment = () => setState(state + 1); + + if (state > 0 && state < 2) { + setState(state + 1); + } + + return
{state}
; + } + + await render(); + expect(scratch.innerHTML).to.equal("
0
"); + + await act(() => { + increment(); + }); + expect(scratch.innerHTML).to.equal("
2
"); + }); + + it("should not fail when a component calls setState multiple times while rendering", async () => { + let increment: () => void; function App() { const [state, setState] = useState(0); - if (state == 0) { - setState(1); + increment = () => setState(state + 1); + + if (state > 0 && state < 5) { + setState(state + 1); } return
{state}
; } await render(); - expect(scratch.innerHTML).to.equal("
1
"); + expect(scratch.innerHTML).to.equal("
0
"); + + await act(() => { + increment(); + }); + expect(scratch.innerHTML).to.equal("
5
"); }); }); diff --git a/packages/react/test/shared/mounting.tsx b/packages/react/test/shared/mounting.tsx index d34603a43..3a28662e4 100644 --- a/packages/react/test/shared/mounting.tsx +++ b/packages/react/test/shared/mounting.tsx @@ -114,7 +114,7 @@ export function mountSignalsTests( expect(html).to.equal("
-20
"); }); - it("should not fail when a component calls setState while rendering", async () => { + it("should not fail when a component calls setState while mounting", async () => { function App() { const [state, setState] = useState(0); if (state == 0) { @@ -127,6 +127,20 @@ export function mountSignalsTests( const html = await render(); expect(html).to.equal("
1
"); }); + + it("should not fail when a component calls setState multiple times while mounting", async () => { + function App() { + const [state, setState] = useState(0); + if (state < 5) { + setState(state + 1); + } + + return
{state}
; + } + + const html = await render(); + expect(html).to.equal("
5
"); + }); }); describe("useSignal()", () => { From 792ad2ecd8cd3ea8b01f2c0c75fba0c241a859a5 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 21 Apr 2023 09:26:27 -0700 Subject: [PATCH 20/26] Run server tests on minify --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 5401d230b..b9f6af5e2 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,7 @@ "postbuild": "node ./scripts/node-13-exports.js", "lint": "eslint 'packages/**/*.{ts,tsx,js,jsx}'", "test": "pnpm test:karma && pnpm test:mocha", - "test:minify": "pnpm test:karma:minify", + "test:minify": "pnpm test:karma:minify && pnpm test:mocha", "test:prod": "pnpm test:karma:prod && pnpm test:mocha:prod", "test:karma": "cross-env COVERAGE=true karma start karma.conf.js --single-run", "test:karma:minify": "cross-env COVERAGE=true MINIFY=true karma start karma.conf.js --single-run", From 1e2f9f39936b072e794781e536f2d386cb342d8d Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 21 Apr 2023 10:11:35 -0700 Subject: [PATCH 21/26] Add test for state-less hooked component --- packages/react/src/index.ts | 9 ---- packages/react/test/browser/updates.test.tsx | 54 ++++++++++++++++++++ 2 files changed, 54 insertions(+), 9 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 1968d6954..6ea0b166f 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -273,15 +273,6 @@ function getDispatcherType(dispatcher: ReactDispatcher | null): DispatcherType { const cached = dispatcherTypeCache.get(dispatcher); if (cached !== undefined) return cached; - // TODO: Explore using something like `useReducer === useCallback` to detect - // ContextOnlyDispatcher. - - // TODO: Consider adding tests for the situation where the dispatcher starts - // out as HooksDispatcherOnMountInDEV and then changes to - // HooksDispatcherOnMountWithHookTypesInDEV. I believe this is possible if a - // component only uses state-less hooks such as useContext which give a fiber - // hook types in DEV but do not give it a memoized state. - // The ContextOnlyDispatcher sets all the hook implementations to a function // that takes no arguments and throws and error. Check the number of arguments // for this dispatcher's useCallback implementation to determine if it is a diff --git a/packages/react/test/browser/updates.test.tsx b/packages/react/test/browser/updates.test.tsx index c50036826..436357eeb 100644 --- a/packages/react/test/browser/updates.test.tsx +++ b/packages/react/test/browser/updates.test.tsx @@ -17,6 +17,8 @@ import { StrictMode, createRef, useState, + useContext, + createContext, } from "react"; import { renderToStaticMarkup } from "react-dom/server"; @@ -380,6 +382,58 @@ describe("@preact/signals-react updating", () => { }); expect(scratch.innerHTML).to.equal("
5
"); }); + + it("should not fail when a component only uses state-less hooks", async () => { + // This test is suppose to trigger a condition in React where the + // HooksDispatcherOnMountWithHookTypesInDEV is used. This dispatcher is + // used in the development build of React if a component has hook types + // defined but no memoizedState, meaning no stateful hooks (e.g. useState) + // are used. `useContext` is an example of a state-less hook because it + // does not mount any hook state onto the fiber's memoizedState field. + // + // However, as of writing, because our react adapter inserts a + // useSyncExternalStore into all components, all components have memoized + // state and so this condition is never hit. However, I'm leaving the test + // to capture this unique behavior to hopefully catch any errors caused by + // not understanding or handling this in the future. + + const sig = signal(0); + const MyContext = createContext(0); + + function Child() { + const value = useContext(MyContext); + return ( +
+ {sig} {value} +
+ ); + } + + let updateContext: () => void; + function App() { + const [value, setValue] = useState(0); + updateContext = () => setValue(value + 1); + + return ( + + + + ); + } + + await render(); + expect(scratch.innerHTML).to.equal("
0 0
"); + + await act(() => { + sig.value++; + }); + expect(scratch.innerHTML).to.equal("
1 0
"); + + await act(() => { + updateContext(); + }); + expect(scratch.innerHTML).to.equal("
1 1
"); + }); }); describe("useSignal()", () => { From 7c1e4a69b515222b22a08e08c475268490fac828 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 21 Apr 2023 10:59:21 -0700 Subject: [PATCH 22/26] Update implementation comments --- packages/react/src/index.ts | 158 ++++++++++++++++++++---------------- 1 file changed, 87 insertions(+), 71 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 6ea0b166f..27cd652a6 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -119,6 +119,74 @@ function usePreactSignalStore(nextDispatcher: ReactDispatcher): EffectStore { return store; } +// In order for signals to work in React, we need to observe what signals a +// component uses while rendering. To do this, we need to know when a component +// is rendering. To do this, we watch the transition of the +// ReactCurrentDispatcher to know when a component is rerendering. +// +// To track when we are entering and exiting a component render (i.e. before and +// after React renders a component), we track how the dispatcher changes. +// Outside of a component rendering, the dispatcher is set to an instance that +// errors or warns when any hooks are called. This behavior is prevents hooks +// from being used outside of components. Right before React renders a +// component, the dispatcher is set to an instance that doesn't warn or error +// and contains the implementations of all hooks. Right after React finishes +// rendering a component, the dispatcher is set to the erroring one again. This +// erroring dispatcher is called the `ContextOnlyDispatcher` in React's source. +// +// So, we watch the getter and setter on `ReactCurrentDispatcher.current` to +// monitor the changes to the current ReactDispatcher. When the dispatcher +// changes from the ContextOnlyDispatcher to a "valid" dispatcher, we assume we +// are entering a component render. At this point, we setup our +// auto-subscriptions for any signals used in the component. We do this by +// creating an Signal effect and manually starting the Signal effect. We use +// `useSyncExternalStore` to trigger rerenders on the component when any signals +// it uses changes. +// +// When the dispatcher changes from a valid dispatcher back to the +// ContextOnlyDispatcher, we assume we are exiting a component render. At this +// point we stop the effect. +// +// Some additional complexities to be aware of: +// - If a component calls `setState` while rendering, React will re-render the +// component immediately. Before triggering the re-render, React will change +// the dispatcher to the HooksDispatcherOnRerender. When we transition to this +// rerendering adapter, we need to re-trigger our hooks to keep the order of +// hooks the same for every render of a component. +// +// - In development, useReducer, useState, and useMemo change the dispatcher to +// a different warning dispatcher (not ContextOnlyDispatcher) before invoking +// the reducer and resets it right after. +// +// The useSyncExternalStore shim will use some of these hooks when we invoke +// it while entering a component render. We need to prevent this dispatcher +// change caused by these hooks from re-triggering our entering logic (it +// would cause an infinite loop if we did not). We do this by using a lock to +// prevent the setter from running while we are in the setter. +// +// When a Component's function body invokes useReducer, useState, or useMemo, +// this change in dispatcher should not signal that we are entering or exiting +// a component render. We ignore this change by detecting these dispatchers as +// different from ContextOnlyDispatcher and other valid dispatchers. +// +// - The `use` hook will change the dispatcher to from a valid update dispatcher +// to a valid mount dispatcher in some cases. Similarly to useReducer +// mentioned above, we should not signal that we are exiting a component +// during this change. Because these other valid dispatchers do not pass the +// ContextOnlyDispatcher check, they do not affect our logic. +// +// - When server rendering, React does not change the dispatcher before and +// after each component render. It sets it once for before the first render +// and once for after the last render. This means that we will not be able to +// detect when we are entering or exiting a component render. This is fine +// because we don't need to detect this for server rendering. A component +// can't trigger async rerenders in SSR so we don't need to track signals. +// +// If a component updates a signal value while rendering during SSR, we will +// not rerender the component because the signal value will synchronously +// change so all reads of the signal further down the tree will see the new +// value. + /* PROD ReactCurrentDispatcher transition diagram (does not include dev time warning dispatchers which are just always ignored): @@ -175,51 +243,6 @@ const dispatcherMachinePROD = createMachine({ ``` */ -// TODO: Update. -// -// To track when we are entering and exiting a component render (i.e. before and -// after React renders a component), we track how the dispatcher changes. -// Outside of a component rendering, the dispatcher is set to an instance that -// errors or warns when any hooks are called. This behavior is prevents hooks -// from being used outside of components. Right before React renders a -// component, the dispatcher is set to a valid one. Right after React finishes -// rendering a component, the dispatcher is set to an erroring one again. This -// erroring dispatcher is called the `ContextOnlyDispatcher` in React's source. -// -// So, we watch the getter and setter on `ReactCurrentDispatcher.current` to -// monitor the changes to the current ReactDispatcher. When the dispatcher -// changes from the ContextOnlyDispatcher to a valid dispatcher, we assume we -// are entering a component render. At this point, we setup our -// auto-subscriptions for any signals used in the component. We do this by -// creating an effect and manually starting the effect. We use -// `useSyncExternalStore` to trigger rerenders on the component when any signals -// it uses changes. -// -// When the dispatcher changes from a valid dispatcher back to the -// ContextOnlyDispatcher, we assume we are exiting a component render. At this -// point we stop the effect. -// -// Some edge cases to be aware of: -// - In development, useReducer, useState, and useMemo changes the dispatcher to -// a different warning dispatcher (not ContextOnlyDispatcher) before invoking -// the reducer and resets it right after. -// -// The useSyncExternalStore shim will use some of these hooks when we invoke -// it while entering a component render. We need to prevent this dispatcher -// change caused by these hooks from re-triggering our entering logic (it -// would cause an infinite loop if we did not). We do this by using a lock to -// prevent the setter from running while we are in the setter. -// -// When a Component's function body invokes useReducer, useState, or useMemo, -// this change in dispatcher should not signal that we are exiting a component -// render. We ignore this change by detecting these dispatchers as different -// from ContextOnlyDispatcher and other valid dispatchers. -// -// - The `use` hook will change the dispatcher to from a valid update dispatcher -// to a valid mount dispatcher in some cases. Similarly to useReducer -// mentioned above, we should not signal that we are exiting a component -// during this change. Because these other valid dispatchers do not pass the -// ContextOnlyDispatcher check, they do not affect our logic. let lock = false; let currentDispatcher: ReactDispatcher | null = null; Object.defineProperty(ReactInternals.ReactCurrentDispatcher, "current", { @@ -262,9 +285,6 @@ const ServerDispatcherType = 1 << 5; const BrowserClientDispatcherType = MountDispatcherType | UpdateDispatcherType | RerenderDispatcherType; -// We inject a useSyncExternalStore into every function component via -// CurrentDispatcher. This prevents injecting into anything other than a -// function component render. const dispatcherTypeCache = new Map(); function getDispatcherType(dispatcher: ReactDispatcher | null): DispatcherType { // Treat null the same as the ContextOnlyDispatcher. @@ -274,19 +294,16 @@ function getDispatcherType(dispatcher: ReactDispatcher | null): DispatcherType { if (cached !== undefined) return cached; // The ContextOnlyDispatcher sets all the hook implementations to a function - // that takes no arguments and throws and error. Check the number of arguments - // for this dispatcher's useCallback implementation to determine if it is a - // ContextOnlyDispatcher. All other dispatchers, warning or not, define - // functions with arguments and so fail this check. For these dispatchers, we - // check the implementation for signifiers indicating what kind of dispatcher - // it is. + // that takes no arguments and throws and error. This dispatcher is the only + // dispatcher where useReducer and useEffect will have the same + // implementation. let type: DispatcherType; const useCallbackImpl = dispatcher.useCallback.toString(); if (dispatcher.useReducer === dispatcher.useEffect) { type = ContextOnlyDispatcherType; - // @ts-expect-error Only in server renderers is this true which the types - // will never represent. + // @ts-expect-error When server rendering, useEffect and useImperativeHandle + // are both set to noop functions and so have the same implementation. } else if (dispatcher.useEffect === dispatcher.useImperativeHandle) { type = ServerDispatcherType; } else if (/Invalid/.test(useCallbackImpl)) { @@ -296,12 +313,12 @@ function getDispatcherType(dispatcher: ReactDispatcher | null): DispatcherType { } else if ( // The development mount dispatcher invokes a function called // `mountCallback` whereas the development update/re-render dispatcher - // invokes a function called `updateCallback`. Use that to determine if we - // are in a mount or update-like dispatcher in development. The production - // mount dispatcher defines an array of the form [callback, deps] whereas - // update/re-render dispatchers read the array using array indices (e.g. - // `[0]` and `[1]`). Use those differences to determine if we are in a mount - // or update-like dispatcher in production. + // invokes a function called `updateCallback`. Use that difference to + // determine if we are in a mount or update-like dispatcher in development. + // The production mount dispatcher defines an array of the form [callback, + // deps] whereas update/re-render dispatchers read the array using array + // indices (e.g. `[0]` and `[1]`). Use those differences to determine if we + // are in a mount or update-like dispatcher in production. /updateCallback/.test(useCallbackImpl) || (/\[0\]/.test(useCallbackImpl) && /\[1\]/.test(useCallbackImpl)) ) { @@ -357,17 +374,16 @@ function isEnteringComponentRender( // entering a new component render. return false; } else if (nextDispatcherType & RerenderDispatcherType) { - // Any transition into the rerender dispatcher is a rerender is the - // beginning of a component render, so we should invoke our hooks. Details - // below. + // Any transition into the rerender dispatcher is the beginning of a + // component render, so we should invoke our hooks. Details below. // - // ## In-place rerendering on mount (Mount -> Rerender) + // ## In-place rerendering (e.g. Mount -> Rerender) // - // If we are transitioning from the mount, update, or rerender dispatcher to the rerender - // dispatcher (e.g. HooksDispatcherOnMount to HooksDispatcherOnRerender), - // then this component is rerendering due to calling setState inside of its - // function body. We are re-entering a component's render method and so we - // should re-invoke our hooks. + // If we are transitioning from the mount, update, or rerender dispatcher to + // the rerender dispatcher (e.g. HooksDispatcherOnMount to + // HooksDispatcherOnRerender), then this component is rerendering due to + // calling setState inside of its function body. We are re-entering a + // component's render method and so we should re-invoke our hooks. return true; } else { // ## Resuming suspended mount edge case (Update -> Mount) From 5a37285cfc0a4e710d188e50369a8ebe377135f3 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 21 Apr 2023 10:59:31 -0700 Subject: [PATCH 23/26] Add test for changing signal during mount --- packages/react/test/shared/mounting.tsx | 22 ++++++++++++++++++++++ packages/react/test/shared/utils.ts | 2 ++ 2 files changed, 24 insertions(+) diff --git a/packages/react/test/shared/mounting.tsx b/packages/react/test/shared/mounting.tsx index 3a28662e4..79dfe1aa9 100644 --- a/packages/react/test/shared/mounting.tsx +++ b/packages/react/test/shared/mounting.tsx @@ -158,5 +158,27 @@ export function mountSignalsTests( const html = await render(); expect(html).to.equal("
1
"); }); + + it("should properly update signal values changed during mount", async () => { + function App() { + const count = useSignal(0); + if (count.value == 0) { + count.value++; + } + + return ( +
+ {count} + +
+ ); + } + + const html = await render(); + expect(html).to.equal("
1
"); + + const html2 = await render(); + expect(html2).to.equal("
1
"); + }); }); } diff --git a/packages/react/test/shared/utils.ts b/packages/react/test/shared/utils.ts index 7a6509a08..e282deb43 100644 --- a/packages/react/test/shared/utils.ts +++ b/packages/react/test/shared/utils.ts @@ -103,6 +103,8 @@ const messagesToIgnore = [ /async tests and hooks,/, // Ignore React 16 warnings about awaiting `act` calls (warning removed in React 18) /Do not await the result of calling act/, + // Ignore how chai or mocha uses `console.error` to print out errors + /AssertionError/, ]; export function checkConsoleErrorLogs(): void { From 492bddd1cdc040cb84c2aa454d1a88b78ffad86c Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 21 Apr 2023 11:02:36 -0700 Subject: [PATCH 24/26] Update comment on state machine --- packages/react/src/index.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 27cd652a6..f09883fd1 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -188,8 +188,13 @@ function usePreactSignalStore(nextDispatcher: ReactDispatcher): EffectStore { // value. /* -PROD ReactCurrentDispatcher transition diagram (does not include dev time -warning dispatchers which are just always ignored): +Below is a state machine definition for transitions between the various +dispatchers in React's prod build. (It does not include dev time warning +dispatchers which are just always ignored). + +ENTER and EXIT suffixes indicates whether this ReactCurrentDispatcher transition +signals we are entering or exiting a component render, or if it doesn't signal a +change in the component rendering lifecyle (NOOP). ```js // Paste this into https://stately.ai/viz to visualize the state machine. From 21c8ee98070a8bda05095dc91b64d2fe54042fb3 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 21 Apr 2023 11:10:25 -0700 Subject: [PATCH 25/26] Add changeset --- .changeset/lazy-badgers-happen.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/lazy-badgers-happen.md diff --git a/.changeset/lazy-badgers-happen.md b/.changeset/lazy-badgers-happen.md new file mode 100644 index 000000000..2f876a261 --- /dev/null +++ b/.changeset/lazy-badgers-happen.md @@ -0,0 +1,5 @@ +--- +"@preact/signals-react": patch +--- + +Fix React adapter in SSR and when rerendering From eba0b934675da18a7441f7f8bbe5a50806b5c86b Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Fri, 21 Apr 2023 11:12:43 -0700 Subject: [PATCH 26/26] Update CI to build then test --- .github/workflows/main.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 0b073c858..fb8f5f5e9 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -42,12 +42,12 @@ jobs: - name: Install dependencies run: pnpm install - - name: Tests - run: pnpm test - - name: Build run: pnpm build + - name: Tests + run: pnpm test + - name: Test production build run: pnpm test:minify